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

add gasLimitMultiplier to fireblocks and privatekey wallets #165

Closed
wants to merge 2 commits into from

Conversation

samlaf
Copy link
Collaborator

@samlaf samlaf commented Mar 28, 2024

Fixes errors we were getting on avs-sync with txs reverting due to running out of gas.

@ian-shim this is a super simple gasLimitMultiplier parameter that the user can set (so we can test whether 1.1, 1.2, etc works best for different cases). hardcoded the default as 1.1

@samlaf samlaf requested a review from ian-shim March 28, 2024 18:37
Comment on lines +216 to +217
return nil, types.WrapError(fmt.Errorf("Failed to get operators state at block %d, quorums %v and registryCoordinatorAddr %v",
curBlock, quorumNumbers, r.registryCoordinatorAddr), err)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

throwing this into this PR if you don't mind

@samlaf
Copy link
Collaborator Author

samlaf commented Mar 28, 2024

Realizing we prob want this logic in the txmgr instead.... will create another PR.

EDIT: there are a few options here, so will wait for your feedback:

  1. wallets automatically bump by 10% (what this PR is doing)
  2. wallets have a new function SendTxWithGasBump(buffer float64)
  3. txMgr's send() function will bump by 10% before sending to wallet.send()
  4. txMgr will have a new function sendWithGasBump

@ian-shim
Copy link
Contributor

Realizing we prob want this logic in the txmgr instead.... will create another PR.

EDIT: there are a few options here, so will wait for your feedback:

  1. wallets automatically bump by 10% (what this PR is doing)
  2. wallets have a new function SendTxWithGasBump(buffer float64)
  3. txMgr's send() function will bump by 10% before sending to wallet.send()
  4. txMgr will have a new function sendWithGasBump

I think wallet should remain simple/minimal interface to interact with chain. The complexity for handling gas cost, replacing stalled transactions, etc. should be the responsibility of the caller. I think option 3 with configureable gas bumps is the most preferable

@samlaf
Copy link
Collaborator Author

samlaf commented Mar 28, 2024

Realizing we prob want this logic in the txmgr instead.... will create another PR.
EDIT: there are a few options here, so will wait for your feedback:

  1. wallets automatically bump by 10% (what this PR is doing)
  2. wallets have a new function SendTxWithGasBump(buffer float64)
  3. txMgr's send() function will bump by 10% before sending to wallet.send()
  4. txMgr will have a new function sendWithGasBump

I think wallet should remain simple/minimal interface to interact with chain. The complexity for handling gas cost, replacing stalled transactions, etc. should be the responsibility of the caller. I think option 3 with configureable gas bumps is the most preferable

sgtm! Closing in favor of #167

@samlaf samlaf closed this Mar 28, 2024
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.

2 participants