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

fix: correct issue where gas simulation can't be made with --generate-only flag #126

Merged
merged 2 commits into from
Mar 9, 2022

Conversation

ejfitzgerald
Copy link
Member

@ejfitzgerald ejfitzgerald commented Mar 8, 2022

Updated SDK logic to correctly query the account number and sequence when simulating transactions using the --generate-only flag

daeMOn63
daeMOn63 previously approved these changes Mar 8, 2022
Copy link
Contributor

@daeMOn63 daeMOn63 left a comment

Choose a reason for hiding this comment

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

Checked v0.45 and it doesn't have the bug (they've updated CalculateGas)
Otherwise lgtm, not really sure why updatedTxf was introduced, it doesn't seem necessary at first sight, should be ok to assign back to txf directly

@ejfitzgerald
Copy link
Member Author

ejfitzgerald commented Mar 8, 2022

Checked v0.45 and it doesn't have the bug (they've updated CalculateGas) Otherwise lgtm, not really sure why updatedTxf was introduced, it doesn't seem necessary at first sight, should be ok to assign back to txf directly

This was an absolute head scratcher for me (and maybe just illustrates my poor understanding of go). When I made the original update I did not introduce updatedTxf, however, this resulted in the calculated gas value not being added to the generated tx. Some Printf-ing later it was the case that the txf here:

https://github.com/fetchai/cosmos-sdk/pull/126/files#diff-51a03bdc40c78352a7e2db8c850e9d2611c70440df997ee92a03215528192b32R66

Seemed to go out of scope and then when the final tx is built (see link below) it used to the original value from the function parameters:

https://github.com/fetchai/cosmos-sdk/pull/126/files#diff-51a03bdc40c78352a7e2db8c850e9d2611c70440df997ee92a03215528192b32R71

(Checked by looking at the underlying pointer values)

Would you expect this @daeMOn63 ?

client/tx/tx.go Outdated
if txf.SimulateAndExecute() {
if clientCtx.Offline {
return errors.New("cannot estimate gas in offline mode")
}

// If we are simulating the transaction, we are not in offline mode. Then we need to lookup correct sequence
// number and account number information. Otherwise it will fail
txf, err := PrepareFactory(clientCtx, txf)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ejfitzgerald thanks for the feedback, I overlooked it and this is why you needed updatedTxf:
The := operator will create variable in current scope and assign to them. Changing to:

Suggested change
txf, err := PrepareFactory(clientCtx, txf)
var err error
txf, err = PrepareFactory(clientCtx, txf)

make it reuse the txf from the parent scope. But then we have to declare err since it doesn't exist yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes of course. I just didn't spot that! Nice one

@ejfitzgerald ejfitzgerald merged commit b5cd0bf into master Mar 9, 2022
@ejfitzgerald ejfitzgerald deleted the fix/generate-flag-simulation branch March 9, 2022 19:52
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.

3 participants