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

Remove claim module #1301

Merged
merged 4 commits into from
Apr 22, 2022
Merged

Remove claim module #1301

merged 4 commits into from
Apr 22, 2022

Conversation

faddat
Copy link
Member

@faddat faddat commented Apr 20, 2022 •
edited
Loading

Closes: #903

This PR:

  • Removes the claim module
  • changes the module path of osmosis to github.com/osmosis-labs/osmosis/v8

@codecov-commenter
Copy link

Codecov Report

Merging #1301 (7a23f4e) into main (b6d342a) will decrease coverage by 1.16%.
The diff coverage is 9.32%.

@@            Coverage Diff             @@
##             main    #1301      +/-   ##
==========================================
- Coverage   20.90%   19.74%   -1.17%     
==========================================
  Files         196      192       -4     
  Lines       25425    26395     +970     
==========================================
- Hits         5316     5212     -104     
- Misses      19118    20212    +1094     
+ Partials      991      971      -20
Impacted Files Coverage Δ
x/epochs/abci.go 100.00% <ø> (ø)
x/epochs/client/cli/query.go 0.00% <ø> (ø)
x/epochs/genesis.go 100.00% <ø> (ø)
x/epochs/handler.go 0.00% <ø> (ø)
x/epochs/keeper/epoch.go 57.89% <ø> (ø)
x/epochs/keeper/keeper.go 62.50% <ø> (ø)
x/epochs/module.go 48.64% <ø> (ø)
x/gamm/client/cli/tx.go 60.19% <ø> (+5.09%) ⬆️
x/gamm/handler.go 5.88% <ø> (ø)
x/gamm/keeper/grpc_query.go 42.95% <ø> (ø)
... and 96 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1976a7d...7a23f4e. Read the comment docs.

@alexanderbez alexanderbez requested review from alexanderbez, ValarDragon and xBalbinus and removed request for alexanderbez, ValarDragon and xBalbinus April 20, 2022 13:24
@sunnya97
Copy link
Collaborator

Should we prune all the claim module's state?

@ValarDragon
Copy link
Member

Awesome! In the upgrade handler logic, https://github.com/osmosis-labs/osmosis/blob/main/app/app.go#L424-L432
we need to add a store upgrade for v8, that makes the claim module deleted. That will remove this from state.

We should check that IBC channels still work gracefully across the upgrade, but I suspect it should be fine, since module additions work just fine.

@ValarDragon ValarDragon mentioned this pull request Apr 20, 2022
Merged
@alexanderbez
Copy link
Contributor

@faddat mind resolving the conflicts and we can get this merged pls?

@ValarDragon
Copy link
Member

Lets get this merged after #1304 , and do the deletion from the store list in this PR as well

@faddat
Copy link
Member Author

faddat commented Apr 21, 2022

Happy to

ValarDragon
ValarDragon approved these changes Apr 22, 2022
View reviewed changes
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.

Added the claims deletion logic!

Now LGTM, thanks for making this PR!

@ValarDragon ValarDragon merged commit fe1b5d2 into main Apr 22, 2022
@ValarDragon ValarDragon deleted the faddat/remove-claims-v2 branch April 22, 2022 04:36
ValarDragon
ValarDragon reviewed May 26, 2022
View reviewed changes
cmd/osmosisd/cmd/balances_from_state_export.go.history
@@ -257,34 +255,6 @@ Example:
snapshotAccs[address] = acc
}

claimGenesis := claimtypes.GenesisState{}
Copy link
Member

Choose a reason for hiding this comment

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

Oh this should not have been moved to a history file. We need to undo this

@ValarDragon ValarDragon mentioned this pull request May 26, 2022
Closed
ValarDragon
ValarDragon reviewed May 31, 2022
View reviewed changes
cmd/osmosisd/cmd/balances_from_state_export.go.history
@@ -313,8 +283,6 @@ Example:
Add(sdk.NewCoin(appparams.BaseCoinUnit, account.Staked)).
Add(sdk.NewCoin(appparams.BaseCoinUnit, account.UnbondingStake)).
Add(account.Bonded...).
Add(account.UnclaimedAirdrop...)
snapshotAccs[addr] = account
Copy link
Member

Choose a reason for hiding this comment

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

Oh no, this was an error that was missed as well :/

Copy link
Contributor

@alexanderbez alexanderbez May 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What do you mean error? I removed it in the backport branch. Do we need to add it back?

Copy link
Member

Choose a reason for hiding this comment

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

yeah we needed to still run snapshotAccs[addr] = account to update the value in the map, thats what get marshalled

@ValarDragon ValarDragon mentioned this pull request May 31, 2022
Merged
ValarDragon added a commit that referenced this pull request May 31, 2022
@ValarDragon
Fix bug in the airdrop claim script, introduced from #1301
84acf1a
ValarDragon added a commit that referenced this pull request May 31, 2022
@ValarDragon @alexanderbez
Fix bug in airdrop script from claims module removal (#1629)
99419b1 
* Fix bug in airdrop script from claims module removal

* Fix bug in the airdrop claim script, introduced from #1301

Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ValarDragon ValarDragon ValarDragon approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

@xBalbinus xBalbinus xBalbinus approved these changes

Assignees
No one assigned
Labels
C:CLI C:x/claim
Projects
Osmosis Chain Development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move claims module to its own repository
6 participants
@faddat @codecov-commenter @sunnya97 @ValarDragon @alexanderbez @xBalbinus