-
Notifications
You must be signed in to change notification settings - Fork 79
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
feat: gas offset param for sendTransaction and sendUserOp #474
feat: gas offset param for sendTransaction and sendUserOp #474
Conversation
…into feat/override_gas_values_for_sendTransaction
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
V nice. Had one point regarding using an 'offsetFactor' instead of 'offsetIncrement'. Any thoughts?
…into feat/override_gas_values_for_sendTransaction
was there a strong demand for this feature? |
No strong demand but some clients did need this, they wanted to override gas values and he didnt offer this in the SDK when using sendTransaction method, so they had to use sendUserOperation, I don't remember the client name. |
This is an optional feature in the sdk, i personally faced this issue while running some scripts that gas estimations from paymaster, especially callGasLimit was low and was causing internal transaction failure. I had to fallback to using buildUserOp and sendUserOp flow, so i can change these gasValues before signing and sending UserOp. So while we can handle most of the use case, we can't handle all edge cases. If some dapps is facing this issue, there should be an easy way for them to handle this use case without manually handling userOp on their end. |
@arcticfloyd1984 Can you confirm if the actual implementation is correct then ? I might just release this in the end if it does no harm but it can be useful to clients. Before releasing I will be removing maxFeePerGas and maxPriorityFeePerGas tho, as increasing this will do no good. |
I am good with the implementation @VGabriel45 |
Shouldn't be possible to decrease the values using this PR's approach, you can only increase values by a percentage, cannot override them. |
exactly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah looks great, nice job
* feat: gas offset param for sendTransaction and sendUserOp (#474) * feat - gas offset param for sendTransaction * added percentage based gas offsets * added dummyPndOverride * refactor gas offset percentage * refactor names and ts doc + lint * improved tsdoc & fixed comments * refactor sendTransaction example tsdoc comment * refactor sendTransaction test --------- Co-authored-by: GabiDev <gv@popoo.io> * feat: transfer ownership (#484) feat: transfer ownership (#484) * feat: transfer_ownership_v2 (#488) * feat/transfer_ownership(in progress) * cleanup * fix linting * added test for transferOwnership with session key manager module * Fix linting * refactor: refactor based on PR review * improve ts doc + refactor tests * added "moduleAddress" param to transferOwnership() * fix module tests * removed console.logs * fixed lint + removed unused import * remove unused import * added argument type for module address --------- Co-authored-by: GabiDev <gv@popoo.io> --------- Co-authored-by: GabiDev <gv@popoo.io>
* feat: gas offset param for sendTransaction and sendUserOp (#474) * feat - gas offset param for sendTransaction * added percentage based gas offsets * added dummyPndOverride * refactor gas offset percentage * refactor names and ts doc + lint * improved tsdoc & fixed comments * refactor sendTransaction example tsdoc comment * refactor sendTransaction test --------- Co-authored-by: GabiDev <gv@popoo.io> * feat: transfer ownership (#484) feat: transfer ownership (#484) * feat: transfer_ownership_v2 (#488) * feat/transfer_ownership(in progress) * cleanup * fix linting * added test for transferOwnership with session key manager module * Fix linting * refactor: refactor based on PR review * improve ts doc + refactor tests * added "moduleAddress" param to transferOwnership() * fix module tests * removed console.logs * fixed lint + removed unused import * remove unused import * added argument type for module address --------- Co-authored-by: GabiDev <gv@popoo.io> * chore: release v4.3.0 (#491) * release v4.3.0 * refactor: increase timeout for transferOwnership tests --------- Co-authored-by: GabiDev <gv@popoo.io> --------- Co-authored-by: GabiDev <gv@popoo.io> Co-authored-by: Joe Pegler <joepegler123@gmail.com>
Summary
Implemented a gas offset parameter for utilization with sendTransaction & buildUserOp + sendUserOp.
This additional parameter enables the augmentation of gas values previously estimated through Biconomy's infrastructure.
When employing a paymaster and specifying a gas offset, calculateGasLimits will be disabled to prevent the paymaster's gas estimations from overriding your offset.
Change Type
Checklist
PR-Codex overview
This PR updates package configurations and refactors functions related to gas calculations and user operations.
Detailed summary
package.json
files configurationBiconomyPaymaster.ts
Utils.ts
Types.ts
with new options for building user operations