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

Separate handling of read and write contract functions #46

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vishal-k25
Copy link

This pull request introduces changes by handling the function calls by differentiating between read (view/pure) and write functions.


constructor(address: string, abi: string, signer: VerifiedWallet | Signer) {
this.signer = signer;
this.abiInterface = new utils.Interface(abi)
this.contract = new ethers.Contract(address, this.abiInterface, signer);
}

private isReadFunction(functionName: string, ...args: any): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

...args can be removed here

@@ -34,10 +34,13 @@
"@biconomy/account": "^4.1.1",
"ethereumjs-tx": "^2.1.2",
"ethers": "^5.7.2",
"tslib": "^2.6.2"
"sinon": "^18.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

i understand sinon is needed for your testcase, we can't add new dependencies to sdk especially the ones we can do without, we need to remove sinon and your testcase now that your it runs without errors. viem has been updated as well to "viem": "^2.12.0" on my recent update

Copy link
Author

@vishal-k25 vishal-k25 May 24, 2024

Choose a reason for hiding this comment

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

okay I removed sinon. it works without error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nice work with the test you can remove it and edit src/test/readFunctions.spec.ts to add more read functions(doesn't have to base sepolia anymore and you can monitor the console logs to ensure read function didn't call userOp)

Copy link
Author

@vishal-k25 vishal-k25 May 24, 2024

Choose a reason for hiding this comment

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

I run tests for other read functions. no errors, it works perfectly fine without calling userOp.
How many read functions to include in the test?

},
"devDependencies": {
"@types/mocha": "^10.0.6",
"@types/sinon": "^17.0.3",
Copy link
Collaborator

Choose a reason for hiding this comment

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

sinon needs to be removed here too

Copy link
Collaborator

@Mohzcrea8me Mohzcrea8me left a comment

Choose a reason for hiding this comment

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

Nice work, the test ran as expected too. few changes are required before merging @vishal-k25

@Mohzcrea8me Mohzcrea8me added the enhancement New feature or request label May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants