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

feat!: add force withdrawal migration to remove liquidity module on Gaia #30

Open
wants to merge 11 commits into
base: release/v1.6.x
Choose a base branch
from

Conversation

dongsam
Copy link
Collaborator

@dongsam dongsam commented Apr 27, 2023

Description

Force Withdrawal:

  • first step : forcefully withdraw all pool coins of holders that exist in the Cosmos Hub except for ibc escrowed coins, and delete all pool states, This would be done during the first upgrade, which could be Gaia v10.
  • second step : completely remove the liquidity module on the codebase and app during the second upgrade, which could be Gaia v11.
  • pros : remaining pool coins can be withdrawn/burned as much as possible.
  • cons : forced withdrawal is an artificial action that changes the account balance and requires extensive testing and simulations.
  • For accounts that may be module accounts because they don't have Pubkey like the IBC escrow acc, pool coins should not be forcefully withdrawn as this may cause problems when the pool coins return via IBC.

Progress ( based sdk 0.45.x )

  • bump ConsensusVersion from 2 to 3 for migration
  • bump cosmos-sdk, tendermint for gaia v10
  • add force withdrawal logic on migration
  • add test cases
  • simulation
  • handling remaining reserve coins on module accs after force withdrawal (community fund)
  • delete all pools and pool batches

Simulation

liquidity module force withdrawal simulation result (cosmos-hub state height 14922680 based)

Decision

  • It is necessary to decide how to handle the reserve coins for the pool coins remaining in the 10 module accounts after the forced withdrawal, whether to send them to the community fund or leave them at the reserve addresses
  • -> Decided to send to community fund in this issue

Ref


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2023

Codecov Report

❗ No coverage uploaded for pull request base (release/v1.6.x@c2fad97). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head cfb7189 differs from pull request most recent head 330ce8f. Consider uploading reports for the commit 330ce8f to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@                Coverage Diff                @@
##             release/v1.6.x      #30   +/-   ##
=================================================
  Coverage                  ?   33.45%           
=================================================
  Files                     ?       44           
  Lines                     ?    12601           
  Branches                  ?        0           
=================================================
  Hits                      ?     4216           
  Misses                    ?     7888           
  Partials                  ?      497           

@dongsam dongsam marked this pull request as ready for review May 25, 2023 07:38
@sainoe
Copy link

sainoe commented Jun 13, 2023

Thanks @dongsam! Just have a few comments on some potential simplification and documentation.
Great work!

@dongsam
Copy link
Collaborator Author

dongsam commented Jun 14, 2023

Thank you for your kind and detailed review @sainoe. I left a comment on the review and push the commit that applied the suggestions.

x/liquidity/keeper/migration.go Outdated Show resolved Hide resolved
x/liquidity/keeper/migration.go Outdated Show resolved Hide resolved
x/liquidity/keeper/migration.go Outdated Show resolved Hide resolved
@dongsam dongsam requested a review from hallazzang June 15, 2023 09:46
"failed fund community pool",
"pool id", pool.Id,
"error", err,
)

Choose a reason for hiding this comment

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

if FundCommunityPool failed, no return err ?

Copy link
Collaborator Author

@dongsam dongsam Jun 15, 2023

Choose a reason for hiding this comment

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

Yes, The FundCommunityPool is a very
simple function that is all about Send, Get, Add, and Set. In general, it is difficult to reproduce the failure and the probability of failure is close to zero, so error handling was not performed. Even if all of the previous logic succeeded and failed at the here, rather than reverting the entire migration by returning the error, it can be solved by re-calling only the FundCommunityPool function in the subsequent upgrade, so I don't think it's necessary to revert.

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.

None yet

5 participants