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

Protorev: Update timing for pool updating and dev fee payout #4827

Merged
merged 40 commits into from
May 26, 2023

Conversation

NotJeremyLiu
Copy link
Contributor

@NotJeremyLiu NotJeremyLiu commented Apr 3, 2023

What is the purpose of the change

This PR updates the timing cadence of certain protorev logic:

  1. Changes dev account payment from once per week to after every trade (this is because the accumulation into the module account and taking from it has bad UX for those tracking protorev profits)
  2. Changes highest liquidity pools updating from once per week to daily
  3. Sets up protorev module for v16 upgrade migration

Brief Changelog

  • Uses daily epoch instead of weekly epoch for highest liquidity updating
  • Removes all developer fee logic from epoch triggering into ExecuteTrade triggering
  • Adds an upgrade migration in ProtoRev to payout what is left in the store when the v16 upgrade happens. This store will no longer be used with the new logic
  • Marks (via comments) no longer used functions in v16 as deprecated , and also denotes functions used only for the upgrade as removable in v17

Testing and Verifying

  • Removed tests for no longer used methods
  • Added a test for the new SendDeveloperFee method
  • All other tests pass after setting the developer account in the tests
  • Added checks in the TestUpgrade() function in v16 upgrade_test.go to ensure protorev upgrades properly (transfers all leftover dev fee funds in the store the the dev account)

Documentation and Release Note

  • Changelog added
  • Documentation has been updated to reflect new changes

- Not optimal because it still stores state, but want to show minimal working version
- All tests pass after setting dev account for the tests, no new tests added
@NotJeremyLiu
Copy link
Contributor Author

This commit is a minimal code change version of sending developer profit after every trade: 5bacd9e

It's not optimized for this change since it still stores state to track the dev fee (which is now unnecessary). Will be changed in a future commit to be an optimal version

@NotJeremyLiu NotJeremyLiu added the V:state/breaking State machine breaking PR label Apr 3, 2023
@bpiv400
Copy link

bpiv400 commented Apr 3, 2023

Should we interpret this as unfinished until the unnecessary state tracking is removed?

@NotJeremyLiu
Copy link
Contributor Author

Should we interpret this as unfinished until the unnecessary state tracking is removed?

ye all these draft PRs are kinda not in review state lol, just putting them up as drafts for my own organization

@NotJeremyLiu NotJeremyLiu changed the title Jl/protorev update timing Protorev: Update timing for pool updating and dev fee payout Apr 4, 2023
@NotJeremyLiu NotJeremyLiu added the protorev All things related to x/protorev label Apr 4, 2023
- removes functions no longer necessary
- Still needs upgrade migration work
- Still needs a way to pay current dev fee payment to us during the upgrade
- still needs to get tests to pass
@p0mvn p0mvn mentioned this pull request Apr 10, 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 Apr 20, 2023
@NotJeremyLiu NotJeremyLiu mentioned this pull request Apr 24, 2023
3 tasks
@github-actions github-actions bot added the C:app-wiring Changes to the app folder label Apr 24, 2023
@github-actions github-actions bot removed the Stale label Apr 25, 2023
- When testing via the upgrades_test.go file, the protorev migration would not run since it treated the from and to version to be 2 (pulling from the new consensus version set)
- When adding this line and hardcoding the from version to 1, the upgrade processes as expected
@NotJeremyLiu NotJeremyLiu requested a review from stackman27 May 25, 2023 15:50
app/upgrades/v16/upgrades.go Outdated Show resolved Hide resolved
Comment on lines 133 to 135
if err := keepers.ProtoRevKeeper.SendDeveloperFeesToDeveloperAccount(ctx); err != nil {
ctx.Logger().Error("error sending developer fees to developer account during upgrade, expected in testing, not in prod", "error", err)
}
Copy link
Member

@p0mvn p0mvn May 25, 2023

Choose a reason for hiding this comment

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

I would prefer it if this is handled properly. I know the process for updating e2e is cumbersome. However, handling this in the right way and e2e testing will save us time during the upgrade if any issues occur.

The process for setting this account in e2e is the following:

  1. update e2e genesis for the protorev module in a separate PR:
    err = updateModuleGenesis(appGenState, staketypes.ModuleName, &staketypes.GenesisState{}, updateStakeGenesis)
    if err != nil {
    return err
    }
  2. backport that PR to v15.x
  3. Grab the chain init docker image with v15 tag from here: https://hub.docker.com/r/osmolabs/osmosis-e2e-init-chain/tags
  • should be auto-uploaded
  1. Copy this tag:
    previousVersionOsmoTag = "v15.x-833705e4-1679340262"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@p0mvn @stackman27 The PR for steps 1-2 is ready for review: #5312

If I understand correctly, once the above PR is merged, I will then be able to get the docker tag and then update the tag in this PR, to continue forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

hi @NotJeremyLiu is this PR r4r now. I think just needs rebasing right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stackman27 I just updated the tag and removed the failing test, letting it run through workflow to see if all is good

@@ -24,7 +24,7 @@ const (
// It should be uploaded to Docker Hub. OSMOSIS_E2E_SKIP_UPGRADE should be unset
// for this functionality to be used.
previousVersionOsmoRepository = "osmolabs/osmosis-dev"
previousVersionOsmoTag = "v15.x-833705e4-1679340262"
previousVersionOsmoTag = "v15.x-63e3f053-1685053483"
Copy link
Member

@p0mvn p0mvn May 25, 2023

Choose a reason for hiding this comment

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

please also bump previousVersionInitTag below to v15.x-63e3f053-1685053483

@NotJeremyLiu
Copy link
Contributor Author

All checks pass, finally r4r @stackman27

Copy link
Contributor

@stackman27 stackman27 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM

Comment on lines +36 to 38
// Deprecated: Can be removed in v16
// UpdateDeveloperFees updates the fees that developers can withdraw from the module account
func (k Keeper) UpdateDeveloperFees(ctx sdk.Context, denom string, profit sdk.Int) error {
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove now?

Copy link
Member

Choose a reason for hiding this comment

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

This is unused now. Please submit separate PR if agreed. Not going to block merge on this

@p0mvn p0mvn merged commit 13fd14a into main May 26, 2023
@p0mvn p0mvn deleted the jl/protorev-update-timing branch May 26, 2023 02:10
@stackman27 stackman27 mentioned this pull request May 26, 2023
6 tasks
pysel pushed a commit that referenced this pull request Jun 6, 2023
* Update highest liquidity pools in daily epoch instead of weekly

* Minimal code change to send dev profit after every trade

- Not optimal because it still stores state, but want to show minimal working version
- All tests pass after setting dev account for the tests, no new tests added

* Handles dev fee payment without storing in kvstore

- removes functions no longer necessary
- Still needs upgrade migration work
- Still needs a way to pay current dev fee payment to us during the upgrade
- still needs to get tests to pass

* Increase consensus version

* Revert "Handles dev fee payment without storing in kvstore"

This reverts commit 1f2f3dc.

* add upgrade logic, new functions, and deprecated comments

* add another todo

* Revert "add another todo"

This reverts commit 49c7eaf.

* add another todo

* Panic on migration error

* Hardcode protorev from version to 1

- When testing via the upgrades_test.go file, the protorev migration would not run since it treated the from and to version to be 2 (pulling from the new consensus version set)
- When adding this line and hardcoding the from version to 1, the upgrade processes as expected

* Add tests to ensure protorev upgrade is successful

* Remove deprecated comment to pass linter

* lint

* add changelog entry

* Change documentation to reflect new timing cadence

* Change developer_fees test for new SendDeveloperFee method

- Removes tests for no longer used functions
- Adds a test for the new SendDeveloperFee method

* Update app/upgrades/v16/upgrades.go

fix typo

Co-authored-by: David Terpay <[email protected]>

* move test to top of file, helper at bottom

* fix merge conflict typo

* added migration function to upgrades.go

* bump consensus version to 2

* comment out everything upgrade related to isolate problem

* Revert "comment out everything upgrade related to isolate problem"

This reverts commit da4efca.

* Revert "bump consensus version to 2"

This reverts commit 127dc5b.

* log instead of pass back up errors

* Remove commented out code

* clean up comments

* add clarifying comment

* fix typpo

Co-authored-by: Roman <[email protected]>

* update v15 prev version tag in e2e container

* remove e2e test that checks protorev dev account not initialized

* return err if protorev dev account payment errors during upgrade

* Add dev fee payment check when testing trade execution

---------

Co-authored-by: David Terpay <[email protected]>
Co-authored-by: stackman27 <[email protected]>
Co-authored-by: Roman <[email protected]>
@github-actions github-actions bot mentioned this pull request Feb 15, 2024
@github-actions github-actions bot mentioned this pull request Mar 15, 2024
@github-actions github-actions bot mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring Changes to the app folder C:docs Improvements or additions to documentation protorev All things related to x/protorev V:state/breaking State machine breaking PR
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants