bcnmy / biconomy-client-sdk

Biconomy SDK is a plug & play toolkit for dApps to build transaction legos that enable a highly customised one-click experience for their users
MIT License
73 stars 76 forks source link

feat: gas offset param for sendTransaction and sendUserOp #474

Closed VGabriel45 closed 4 months ago

VGabriel45 commented 4 months ago

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

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}

github-actions[bot] commented 4 months ago

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%)
livingrockrises commented 4 months ago

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

VGabriel45 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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