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.

Sign up for GitHub

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

Jump to bottom

Protorev: Use hooks instead of message parsing to trigger backruns #5045

Merged
merged 76 commits into from
Jun 6, 2023
Merged

Protorev: Use hooks instead of message parsing to trigger backruns #5045

merged 76 commits into from
Jun 6, 2023

Conversation

NotJeremyLiu
Copy link
Contributor

@NotJeremyLiu NotJeremyLiu commented May 2, 2023 •
edited
Loading

What is the purpose of the change

  • We've concluded a hook-based approach to protorev will be the best approach to keep protorev up to date given several new message types being introduced that allow for swaps.
  • Previously, protorev would case on specific message types for a transaction, and extract the pools that were swapped against to check for backrun potential.
  • This PR changes this approach to now plug protorev into the GAMM and Concentrated liquidity hooks, store the necessary swap information in the hook, and then read from that store in the protorev posthandler.
  • We also add logic to support join/exit pool hooks for the GAMM module (treating single sided joins/exits as swaps), and pool creation (updating highest liquidity pools at creation of a new pool)

Brief Changelog

  • Add new kvstore for SwapsToBackrun and add setters and getters
  • Add hooks.go file that plugs into GAMM and ConcratedLiquidity pool hooks and stores/updates necessary protorev kvstores as appropriate based on the hook
    • Add SwapsToBackrun when triggered by a swap hook or join/exit pool hook that also swaps
    • Update highest liquidity pools at pool creation for GAMM and at initial liquidity position for CL (triggered by hook)
  • Add SwapsToBackrun delete logic in postahandler without using gas

Testing and Verifying

  • New tests added for all protorev hook logic in hooks_test.go
  • Updated previous tests to use updated methods and no longer rely on messages as input
  • Updated epoch_hook_test.go to account for new concentrated liquidity pool testing changes that sets default coins when creating a pool with a position range

Documentation and Release Note

  • Added changelog entry

@NotJeremyLiu
Add input and output
02ddd55 
- Follows gamm module patterns such as 1) naming convention and 2) providing tokens (sdk.Coins) even though it's currently expected to be a single token in or out (sdk.Coin).
@NotJeremyLiu
Update modules that use hook
75ea9cc
@NotJeremyLiu
Merge branch 'main' into jl/add-in-and-out-coins-to-cl-swap-listener
d556709
@NotJeremyLiu
Create new kvstore and methods for swpsToBackrun
772af1b 
- With new hook based logic, need to persist the swaps to the posthandler. Doing so via storing in kvstored
@NotJeremyLiu
Connect protorev to cfmm and cl hooks
089d091
@NotJeremyLiu
Implement CFMMHook logic in protorev
1a6ed93
@NotJeremyLiu
Update comments, restrict join/exit pool logic to single denom in/out
dcc5549
@NotJeremyLiu
Remove old placeholder print
616f5dc
@NotJeremyLiu
Change init function to reset
ef2d317
@NotJeremyLiu
add input and output coins in protorev cl hook
9e71597
@NotJeremyLiu
add necessary poolmanager expected keepers
99d8e9b
@NotJeremyLiu
add hook logic for cl pools and refactor
3212d18
@NotJeremyLiu
implement new hook-based logic in the posthandler
9a73b34
@NotJeremyLiu
Merge branch 'main' into jl/protorev-hook-based-logic
d89db40
@NotJeremyLiu
Merge branch 'main' into jl/protorev-hook-based-logic
a1b18ab
@github-actions github-actions bot added C:app-wiring Changes to the app folder C:x/concentrated-liquidity C:x/pool-incentives C:x/twap Changes to the twap module labels May 2, 2023
@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label May 17, 2023
@github-actions github-actions bot removed the Stale label May 19, 2023
@NotJeremyLiu
Add join/exit pool hook tests
fa03d8e 
- Limit len(coins) = 1 due to an exit pool message giving all denoms as exitdenoms (and potentially future behavior of other modules)
@NotJeremyLiu
Add pool creation hook test
cc5f288
@NotJeremyLiu
Add pool creation tests, handle CL pools differently due to 0 liquidi…
8938ba5 
…ty creation
@NotJeremyLiu
Update ExtractSwappedPools to no longer need tx
164615f
@NotJeremyLiu NotJeremyLiu requested a review from p0mvn June 5, 2023 19:37
@p0mvn p0mvn mentioned this pull request Jun 5, 2023
Merged
p0mvn
p0mvn reviewed Jun 5, 2023
View reviewed changes
x/protorev/keeper/hooks.go Outdated Show resolved Hide resolved
p0mvn
p0mvn reviewed Jun 5, 2023
View reviewed changes
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

Great work on expanding the tests! Looks great. I will do a short final pass once all comments are addressed prior to merging

@NotJeremyLiu
Clarify why the test calls DeleteAllPoolsForBaseDenom
e5d17c6
@NotJeremyLiu
Move panic catching into GetComparablePoolLiquidity
22d528e 
- Panic catching was previously in CompareAndStorePool because that function did the multiplication.
- Since then, the multiplication logic that this panic catches overflows on has been separated into it's own function GetComparablePoolLiquidity
- This commit moves the panic catching logic to GetComparablePoolLiquidity
@NotJeremyLiu
Add int overflow panic catching test
ed66bce
@NotJeremyLiu
Revert "Add int overflow panic catching test"
61e7076 
This reverts commit ed66bce.
@NotJeremyLiu
Add int overflow panic catch test - try 2
995b115
@NotJeremyLiu
Add custom error message
026fe77
@NotJeremyLiu
Add overflow catching logical branch tests for CompareAndStorePool
c18ec19
@NotJeremyLiu
Add non-gamm pool error test for StoreJoinExitPoolSwaps()
c2db6d7
@NotJeremyLiu
Use s.SetupTest() and give testAcc pool shares
460da4a
@NotJeremyLiu
Add clarifying comment about gas meter
61d3474
@NotJeremyLiu
Add comment explaining the base denoms check -> CompareAndStorePool l…
8966b6a 
…ogic
p0mvn
p0mvn approved these changes Jun 6, 2023
View reviewed changes
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

Excellent work @NotJeremyLiu

@p0mvn p0mvn merged commit c6697a5 into main Jun 6, 2023
@p0mvn p0mvn deleted the jl/protorev-hook-based-logic branch June 6, 2023 14:35
@github-actions github-actions bot mentioned this pull request Apr 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@czarcas7ic czarcas7ic czarcas7ic left review comments

@stackman27 stackman27 stackman27 approved these changes

@p0mvn p0mvn p0mvn approved these changes

@davidterpay davidterpay davidterpay approved these changes

@nicolaslara nicolaslara Awaiting requested review from nicolaslara

Assignees

@stackman27 stackman27

Labels
C:app-wiring Changes to the app folder protorev All things related to x/protorev V:state/breaking State machine breaking PR
Projects
Osmosis Chain Development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@NotJeremyLiu @stackman27 @p0mvn @davidterpay @czarcas7ic