fix: account for the domain tag when estimating gas - #964
Open
Ruzzgar wants to merge 1 commit into
Open
Conversation
When a domain is passed, getTransactionMethods appends its four byte tag to the calldata inside buildTransaction, and transact sends exactly that. But estimateGas called the contract method with the encoded arguments only, so the tag was missing from what it measured. Calldata is charged per byte, so the returned estimate lands below what the transaction actually consumes. On a validate() call: estimateGas() -> 65562 gasUsed -> 65626 A caller passing the estimate straight through as a gas limit, which is the normal reason to ask for one, runs out of gas. Estimate the transaction buildTransaction produces whenever a domain is set, so the measured calldata is the calldata that gets sent. Calls without a domain keep going through the contract method unchanged. staticCall has the same gap but no visible symptom, since Seaport ignores the trailing bytes and returns the same result either way, so it is left alone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What happened
With a
domainset,buildTransactionappends its four byte tag to the calldata, andtransactsends exactly that.estimateGasnever goes through it, it calls the contract method with the encoded arguments only, so those four bytes are not part of what gets measured.Calldata is charged per byte, so the number comes back short. On a
validate()call:Feed that estimate straight back in as a gas limit, which is the usual reason to ask for one, and the transaction runs out of gas.
The fix
When a domain is set, estimate the transaction
buildTransactionproduces, so the calldata being measured is the calldata being sent. Without a domain the call goes through the contract method exactly as before.Scope
staticCallhas the same gap, but Seaport ignores the trailing bytes and returns the same result either way, so I left it alone rather than change something with no observable effect.Testing
test/estimate-gas.spec.ts: one test asserts the estimate coversgasUsedon a tagged call, one does the same for an untagged call as a control. The first fails onmain, the second passes either way. Suite green at 174.