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 gas meter to tokenfactory trackBeforeSend #5927

Merged
merged 7 commits into from
Aug 1, 2023

Conversation

mattverse
Copy link
Member

Closes: #5725

What is the purpose of the change

Previously, we had a bug in tokenfactory module where registering an infinite contract as send hook contract then calling module to module send would cause infinite loop in the state machine as well, since module to module sends are not properly gas metered.

This PR adds gas meter for track before sned to prevent problem mentioned above.

Testing and Verifying

According tests have been added

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@mattverse mattverse added the V:state/breaking State machine breaking PR label Jul 31, 2023
@mattverse mattverse marked this pull request as draft July 31, 2023 10:19
@mattverse
Copy link
Member Author

Marking as draft since final decision of whether we should suppress the panic happening or not has not been made.

@mattverse mattverse marked this pull request as ready for review July 31, 2023 15:20
em := sdk.NewEventManager()
// if its track before send, apply gas meter to prevent infinite loop
if !blockBeforeSend {
ctx = ctx.WithGasMeter(sdk.NewGasMeter(types.TrackBeforeSendGasLimit))
Copy link
Member

Choose a reason for hiding this comment

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

We want to write this consumed gas meter result to the parent gas meter though. (So if that panics, then its fine)

I think that looks like keeping parentCtx, and doing "parentCtx.GasMeter.Use(childCtx.GasMeter.GasUsed)"

Copy link
Member Author

Choose a reason for hiding this comment

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

If it's because of panic catching, it's already fine as of right now since we recover thread from panic though using defer function above right? (Probably me missing something)

Copy link
Member

@ValarDragon ValarDragon Jul 31, 2023

Choose a reason for hiding this comment

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

Suppose the parent gas meter had say 5 million gas.

It does this + some other stuff. Work here should still contribute to that gas

We still want the gas consumed here to get added to that gas meter. Its totally fine if it can't out of gas error inside this function, the next thing that consumes gas will cause the gas panic

Copy link
Member Author

Choose a reason for hiding this comment

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

Attempt in f8619ff.

@czarcas7ic czarcas7ic mentioned this pull request Jul 31, 2023
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Nice, LGTM!

@ValarDragon ValarDragon merged commit 93086d7 into main Aug 1, 2023
@ValarDragon ValarDragon deleted the mattverse/tokenfacotyr branch August 1, 2023 16:06
@github-actions github-actions bot mentioned this pull request Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/tokenfactory V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tokenfactory before send hooks finalization
2 participants