Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(sdk): operatorContractUtils type safety #2919

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

teogeb
Copy link
Contributor

@teogeb teogeb commented Nov 28, 2024

  • use bigint type when a number is a wei amount
  • use EthereumAddress type for contract addresses
  • use Signer instead of Wallet
    • this allow us to use client.getSigner() as an argument

Other changes

  • rename some methods which are used only for test setup:
    • generateWalletWithGasAndTokens() -> createTestWallet()
    • setupOperatorContract() -> setupTestOperatorContract()
    • deployOperatorContract() -> deployTestOperatorContract()
    • deploySponsorshipContract() -> deployTestSponsorshipContract()
    • getProvider() -> getTestProvider()
  • remove some obsolete parameters from these methods and from getTestAdminWallet() (not used by any component)
  • do not export transferTokens() from operatorContractUtils.ts (it is an internal helper method)

Future improvements

  • Refactor the utilities so that we can use sponsor/stake/delegate etc. methods both for in production environment and in tests
    • e.g. token parameter for transferTokens should be required or injected from the context (now it is implicitly test token)
  • Maybe merge setupTestOperatorContract() and deployTestOperatorContract()
  • Maybe deployTestOperatorContract() could return client's Operator instance?

@teogeb teogeb marked this pull request as ready for review December 17, 2024 13:49
@teogeb teogeb requested a review from harbu December 17, 2024 13:49
Copy link
Contributor

@harbu harbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a strict sense, I don't mind the contract addresses being just plain strings here since EthereumAddress type comes with the lower casing requirement which is not strictly necessary here. However I'm not against using it here either so I'm fine with that change.

}

export async function generateWalletWithGasAndTokens(opts?: GenerateWalletWithGasAndTokensOpts): Promise<Wallet & SignerWithProvider> {
const provider = getProvider()
export async function createTestWallet(): Promise<Wallet & SignerWithProvider> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't always want to spend time waiting for a wallet to be generated that contains tokens, therefore I like the old name since it indicates what kind of wallet we are generating. Sometimes in tests we have wallets that don't need to tokens or gas (just generate random private key), need only gas (use KeyServer) or need both (use this function). I think the name should indicate that we are now generating a certain kind of wallet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would make sense to modify dev-chain-fast so that they KeyServer keys have also some tokens. Then this utility function would be obsolete. https://linear.app/streamr/issue/ETH-800/dev-chain-fast-mint-data-tokens-for-pre-baked-accounts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants