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

feat: gas offset param for sendTransaction and sendUserOp #474

Merged
merged 10 commits into from
May 9, 2024

Conversation

VGabriel45
Copy link
Collaborator

@VGabriel45 VGabriel45 commented Apr 23, 2024

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

  • Bug Fix
  • Refactor
  • New Feature
  • Breaking Change
  • Documentation Update
  • Performance Improvement
  • Other

Checklist

  • [] My code follows this project's style guidelines
  • I've reviewed my own code
  • I've added comments for any hard-to-understand areas
  • I've updated the documentation if necessary
  • My changes generate no new warnings
  • I've added tests that prove my fix is effective or my feature works
  • All unit tests pass locally with my changes
  • Any dependent changes have been merged and published

PR-Codex overview

This PR updates package configurations and refactors functions related to gas calculations and user operations.

Detailed summary

  • Updated package.json files configuration
  • Refactored gas-related functions for user operations
  • Added new gas offset percentage types for calculations
  • Modified gas limit values in BiconomyPaymaster.ts
  • Added new functions for gas calculations in Utils.ts
  • Updated Types.ts with new options for building user operations

The following files were skipped due to too many changes: src/account/BiconomySmartAccountV2.ts, tests/account/write.test.ts

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@VGabriel45 VGabriel45 changed the title feat - gas offset param for sendTransaction feat - gas offset param for sendTransaction & sendUserOp Apr 23, 2024
…into feat/override_gas_values_for_sendTransaction
Copy link

github-actions bot commented Apr 23, 2024

size-limit report 📦

Path Size
core (esm) 52.25 KB (+0.47% 🔺)
core (cjs) 56.87 KB (+0.67% 🔺)
account (tree-shaking) 51.31 KB (+0.7% 🔺)
bundler (tree-shaking) 2.33 KB (0%)
paymaster (tree-shaking) 2.27 KB (+0.65% 🔺)
modules (tree-shaking) 40.05 KB (0%)

@joepegler joepegler changed the title feat - gas offset param for sendTransaction & sendUserOp feat: gas offset param for sendTransaction and sendUserOp Apr 24, 2024
Copy link
Collaborator

@joepegler joepegler left a 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?

src/account/utils/Types.ts Outdated Show resolved Hide resolved
src/account/utils/Types.ts Outdated Show resolved Hide resolved
src/account/utils/Types.ts Outdated Show resolved Hide resolved
src/account/utils/Types.ts Outdated Show resolved Hide resolved
@livingrockrises
Copy link
Collaborator

was there a strong demand for this feature?
if yes which dapps

@VGabriel45
Copy link
Collaborator Author

was there a strong demand for this feature? if yes which dapps

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.
@tomarsachin2271 specifically asked for this also.

@tomarsachin2271
Copy link
Collaborator

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.

@VGabriel45
Copy link
Collaborator Author

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.

@arcticfloyd1984
Copy link
Contributor

I am good with the implementation @VGabriel45
Also, we should not recommend this to anyone unless necessary especially for mainnets as someone can send at a lower preVerificationGas (we currently accept at a 50% lower value than what we estimate on our end).
This can mess up our pricing and make our Bundlers lose money.
Please communicate with everyone involved with client integrations that this should be the last resort after infra has checked why the estimates are not sufficient.

@VGabriel45
Copy link
Collaborator Author

I am good with the implementation @VGabriel45 Also, we should not recommend this to anyone unless necessary especially for mainnets as someone can send at a lower preVerificationGas (we currently accept at a 50% lower value than what we estimate on our end). This can mess up our pricing and make our Bundlers lose money. Please communicate with everyone involved with client integrations that this should be the last resort after infra has checked why the estimates are not sufficient.

Shouldn't be possible to decrease the values using this PR's approach, you can only increase values by a percentage, cannot override them.

@livingrockrises
Copy link
Collaborator

I am good with the implementation @VGabriel45 Also, we should not recommend this to anyone unless necessary especially for mainnets as someone can send at a lower preVerificationGas (we currently accept at a 50% lower value than what we estimate on our end). This can mess up our pricing and make our Bundlers lose money. Please communicate with everyone involved with client integrations that this should be the last resort after infra has checked why the estimates are not sufficient.

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

Copy link
Collaborator

@joepegler joepegler left a 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

@livingrockrises livingrockrises merged commit 73965ab into develop May 9, 2024
6 checks passed
VGabriel45 added a commit that referenced this pull request May 13, 2024
* 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>
VGabriel45 added a commit that referenced this pull request May 14, 2024
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants