-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Problem: estimate less gas when miss max gas on txf when simualte runTx #1303
Conversation
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.
inside of SendMessagesToMempool
we call buildMessages
where we always pass in 0 as the argument for gas
relayer/relayer/chains/cosmos/tx.go
Line 173 in 259b127
txBytes, sequence, fees, err := cc.buildMessages(ctx, msgs, memo, 0, txSignerKey, feegranterKey, sequenceGuard) |
in buildMessages
we call PrepareFactory
which will now return the tx.Factory
with the gas set to the value in MaxGasAmount
if it is a non-zero value.
relayer/relayer/chains/cosmos/tx.go
Lines 610 to 613 in 259b127
txf, err := cc.PrepareFactory(cc.TxFactory(), txSignerKey) | |
if err != nil { | |
return nil, 0, sdk.Coins{}, err | |
} |
further down in buildMessages
we call CalculateGas
if the gas argument passed in is equal to 0 (it always is so CalculateGas
is always invoked.
relayer/relayer/chains/cosmos/tx.go
Lines 628 to 634 in 259b127
if gas == 0 { | |
_, adjusted, err = cc.CalculateGas(ctx, txf, txSignerKey, cMsgs...) | |
if err != nil { | |
return nil, 0, sdk.Coins{}, err | |
} | |
} |
then we reassign the gas value on the tx.Factory
with the value returned from CalculateGas
it would seem then that no matter what we are always going to use the gas value returned from CalculateGas
, so it's not clear to me why this change is necessary but perhaps I'm missing something here.
well typing that all out and taking a deeper look I think I see what the change is intended to do, inside sorry i walked through the call stack twice and it wasnt immediately obvious what this change was doing and the title was slightly confusing. |
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.
lgtm, thanks for the contribution!
No description provided.