-
Notifications
You must be signed in to change notification settings - Fork 607
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
feat(x/protorev): Use Transient store to store swap backruns #7665
Conversation
@@ -11,14 +11,14 @@ | |||
"[go.mod]": { | |||
"editor.formatOnSave": true, | |||
"editor.codeActionsOnSave": { | |||
"source.organizeImports": true | |||
"source.organizeImports": "explicit" |
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.
drive by change, true has been deprecated and has been replaced to explicit
x/protorev/keeper/posthandler.go
Outdated
// // Delete swaps to backrun for next transaction without consuming gas | ||
// // from the current transaction's gas meter, but instead from a new gas meter with 50mil gas. | ||
// // 50 mil gas was chosen as an arbitrary large number to ensure deletion does not run out of gas. | ||
// protoRevDec.ProtoRevKeeper.DeleteSwapsToBackrun(ctx.WithGasMeter(sdk.NewGasMeter(sdk.Gas(50_000_000)))) |
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.
Don't we still need this? Just on the transient store?
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.
Do we? I thought the purpose of Transient stores was that it drops all states anyways so thought we wouldn't need to delete them 🤔
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 does drop all state, but at the end of the block not the end of each tx
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.
(notably these need to get deleted end of each tx, in order to not cause excess backruns next block)
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.
Ahh gotcha, confused myself with this logic being in endblocker. Fixed in 4690908 this commit!
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.
Wow nice, this became quite simple haha
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
Closes: #7551
What is the purpose of the change
Uses Transient stores to store swap backruns instead of using kvstores. This is possible since we're setting store and deleting them on every post handler run !
Testing and Verifying
Existing tests passes
Documentation and Release Note
Unreleased
section ofCHANGELOG.md
?Where is the change documented?
x/{module}/README.md
)