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

Implement clawback of airdrop from inactive genesis addresses #560

Merged
merged 12 commits into from
Nov 19, 2021

Conversation

sunnya97
Copy link
Collaborator

@sunnya97 sunnya97 commented Oct 29, 2021

Closes: #553

Description

Implements the automatic clawback of all OSMO and ION from genesis airdrop recipients at the end of the claim period (6 months after genesis).

For more details, see Osmosis proposal 32: https://www.mintscan.io/osmosis/proposals/32


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2021

Codecov Report

Merging #560 (bd1b2d9) into main (0a105c9) will decrease coverage by 0.11%.
The diff coverage is 57.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #560      +/-   ##
==========================================
- Coverage   20.31%   20.20%   -0.12%     
==========================================
  Files         164      172       +8     
  Lines       23262    24141     +879     
==========================================
+ Hits         4726     4878     +152     
- Misses      17762    18469     +707     
- Partials      774      794      +20     
Impacted Files Coverage Δ
x/claim/keeper/claim.go 63.30% <57.89%> (-0.86%) ⬇️
x/pool-incentives/keeper/grpc_query.go 68.18% <0.00%> (-25.57%) ⬇️
x/incentives/keeper/grpc_query.go 55.29% <0.00%> (-9.79%) ⬇️
x/pool-incentives/keeper/keeper.go 87.09% <0.00%> (-2.91%) ⬇️
x/pool-incentives/types/query.pb.go 15.00% <0.00%> (-2.04%) ⬇️
x/incentives/keeper/gauge.go 66.79% <0.00%> (-1.27%) ⬇️
x/incentives/types/query.pb.go 0.83% <0.00%> (-0.03%) ⬇️
x/epochs/abci.go 100.00% <0.00%> (ø)
x/epochs/genesis.go 100.00% <0.00%> (ø)
x/mint/keeper/keeper.go 63.63% <0.00%> (ø)
... and 14 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 0a105c9...bd1b2d9. Read the comment docs.

Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

Other than the questions asked above, looks lgtm!

x/claim/keeper/claim_test.go Outdated Show resolved Hide resolved
cmd/osmosisd/cmd/genaccounts.go Show resolved Hide resolved
@antstalepresh
Copy link
Contributor

claim module isn't doing this stuff already?

@sunnya97
Copy link
Collaborator Author

sunnya97 commented Nov 1, 2021

@antstalepresh currently the claim module claws back the un-claimed coins from the claims module account (which was 80% of everyone's airdrop amount). But Osmosis governance voted in Prop 32 to also clawback the 20% of OSMO that was in people's accounts as well the IONs for inactive addresses

@sunnya97
Copy link
Collaborator Author

@ValarDragon this should be added to this upgrade, in case we don't get another upgrade in by Dec 18

@mattverse
Copy link
Member

Yep, I marked it as a v5 upgrade checklist, would assign myself to go over the edge case @antstalepresh mentioned within today, tomorrow and get it merged as well!

@ValarDragon
Copy link
Member

ValarDragon commented Nov 16, 2021

Sunny and I discussed today. Its probably not worth resolving that edge case (but a great catch to be aware of). The governance proposal's "letter of the law" interpretation clearly supports pruning the entire account. The "spirit of the proposal" is incredibly unclear, this edge case wasn't spelled out. But theres been no concern from the community about it.

Would rather we focus on other things, instead of resolving it.

@mattverse mattverse self-requested a review November 16, 2021 06:47
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

Cool! In that case, I think this PR is good to go, ready to merge!

x/claim/keeper/claim.go Show resolved Hide resolved
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.

I corrected the test cases. @sunnya97 can you investigate failures. On scan appears to be clawing back from non-airdrop inactive addresses.

Also changes like this require updates to the spec. Please update the concepts.

@sunnya97
Copy link
Collaborator Author

Fixed

@sunnya97 sunnya97 requested a review from ValarDragon November 17, 2021 01:15
Comment on lines 57 to 58
acc := k.accountKeeper.GetAccount(ctx, addr)
if acc != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to make sure I understand the prior bug. Why would it clawback from all accounts? (Or were the addresses just wrong)

Also is there any expected scenario where acc would be nil, or this is just in case something unexpected occurs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it was doing the opposite. It wasn't clawing back from any accounts.
See original testcases:
https://github.com/osmosis-labs/osmosis/blob/cf7ba91f7bd74e6b9e7d3559f5e0e0ff9da87b8b/x/claim/keeper/claim_test.go

It's cause the function was exiting early, b/c as soon as it saw an account that wasn't in the account store it would error.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh that makes sense

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.

Airdrop addrs is a map that we iterate over. I'm concerned over determinism issues here. Lets update to not use a map, but instead a list of addresses. This bool value in the map is not important. Its always true.

Also I made a commit adding more comments to the function, since edge cases, even if determined to not be worth doing, should at least be documented as such.

@ValarDragon
Copy link
Member

On further analysis, this non-determinism has pretty high risk of chain halts. The clawback deletes state entries. IAVL is deletion order dependent.

My understanding is that the delete orders do get sorted by key prior to hitting IAVL in commit, but I'd definitely have to double check, and am quite hesistant to rely on that.

@sunnya97
Copy link
Collaborator Author

Swapped map for an array

@daniel-farina daniel-farina linked an issue Nov 18, 2021 that may be closed by this pull request
4 tasks
@ValarDragon
Copy link
Member

LGTM, A second person should just verify the airdrop accounts (I have not), and then its good to merge.

@ValarDragon
Copy link
Member

Checked the file, its good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add 100% clawback Implement clawback of ALL Osmo and Ion at end of claim period (Prop 32)
7 participants