Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Create and submit update transactions #69
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
Create and submit update transactions #69
Changes from all commits
7c6174f
c280c36
4789669
be121e9
a5c5312
6f4ba44
a0f753d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Fully based on Airseeker v1 implementation. Later we might do #67 but that needs a discussion and agreement first.
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.
Hey, AFAIK in airseeker v1 a configured gas limit is used (gas for calls is not estimated).
Separately, manager-multisig often has to do multicalls and the gas is sometimes incorrectly estimated (often repeatedly for the same transaction). It's just something we should probably keep in mind; if we do estimates will we have a config parameter for adding headroom?
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.
Mhh, are you sure? I based the implementation of the code in the main branch. I can see that we did the estimation here (for all beacons to be updated).
Yeah, the estimating implementation seems a bit off, but I found the reasoning in:
And the estimation function implemented here should have the same behaviour as the one in Airseeker (if not it's my mistake and should be fixed).
cc: @acenolaza since we talked about this a bit on Slack.
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.
There is 10% headroom added when we do estimate of the
multicall
, so I guess that covers the need fortryMulticall
and also adds this headroom.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.
It's possible I made a mistake, thanks for clarifying :)
The current Airseeker implementation works well, so it must be fine :)