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

Add superfluid unbond partial amount #4107

Merged
merged 9 commits into from
Feb 2, 2023

Conversation

t4sk
Copy link
Contributor

@t4sk t4sk commented Jan 26, 2023

What is the purpose of the change

Implement partial unbonding from superfluid staking

Here is an overview of how SuperfluidUndelegateAndUnbondLock works.

  1. Superfluid undelegate all
  2. Superfluid unbond partial amount -> BeginForceUnlock -> Get new "unlocking lock" id
  3. Delete synthetic unlocking lock - undo synthetic lock created in step 1
  4. Create synthetic unlocking lock for underlying lock from step 2
  5. Superfluid re-delegate remaining amount

If the amount to unlock is equal to the underlying locked amount, this function will behave the same as SuperfluidUndelegate followed by SuperfluidUnbondLock. Otherwise this function will create a new synthetic unlocking lock from the newly created underlying lock.

Related links
#4059 (comment)
#3830

Brief Changelog

  • Implements SuperfluidUndelegateAndUnbondLock

Testing and Verifying

This change added tests and can be verified as follows:

(example:)

  • Added unit test inx/superfluid/keeper/stake_test.go

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (yes)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes)
  • How is the feature or change documented? (documented)

@p0mvn
Copy link
Member

p0mvn commented Jan 26, 2023

Thanks for the PR. Can you please add a changelog entry for this?

@p0mvn p0mvn added the V:state/breaking State machine breaking PR label Jan 26, 2023
@t4sk t4sk changed the title Implement superfluid unbond partial amount Add superfluid unbond partial amount Jan 26, 2023
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.

Thanks for the PR! Looks nice 🌮 Left some comments and questions I had + requested some more test cases and checks.

x/superfluid/types/codec.go Outdated Show resolved Hide resolved
proto/osmosis/superfluid/tx.proto Outdated Show resolved Hide resolved
x/superfluid/types/msgs.go Outdated Show resolved Hide resolved
x/superfluid/keeper/stake.go Show resolved Hide resolved
x/superfluid/keeper/stake.go Outdated Show resolved Hide resolved
x/superfluid/keeper/stake.go Show resolved Hide resolved
x/superfluid/keeper/stake.go Show resolved Hide resolved
// setup superfluid delegations
_, intermediaryAccs, locks := suite.setupSuperfluidDelegations(valAddrs, []superfluidDelegation{{0, 0, 0, lockAmount}}, denoms)
suite.checkIntermediaryAccountDelegations(intermediaryAccs)
suite.Require().True(len(locks) > 0)
Copy link
Member

Choose a reason for hiding this comment

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

isn't lock length always = 1 ? If so, can we put it as a variable and then remove the for loop below/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I can. Alternatively would you like me to test with multiple locks?

x/superfluid/keeper/stake_test.go Show resolved Hide resolved
x/superfluid/keeper/stake_test.go Show resolved Hide resolved
x/superfluid/keeper/stake.go Show resolved Hide resolved
x/superfluid/types/codec.go Outdated Show resolved Hide resolved
@mattverse
Copy link
Member

@t4sk Would it be possible to re-assign me as reviewer once you think the PR is ready for a second round of review?

@t4sk
Copy link
Contributor Author

t4sk commented Jan 31, 2023

@mattverse It's ready for another round of review. But I don't have write access to assign reviewers

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.

Once final test case requested has been added, lgtm. Thanks for resolving all the feedbacks in such good quality!

unlockAmount sdk.Int
expectErr bool
splitLockId bool
undelegate bool
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
undelegate bool
undelegating bool

unlockAmount: sdk.NewInt(1),
expectErr: true,
splitLockId: false,
undelegate: true,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
undelegate: true,
undelegating: true,

unbond: false,
},
{
name: "undelegate and unbond an undelegated lock",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name: "undelegate and unbond an undelegated lock",
name: "undelegate and unbond an undelegating lock",

suite.Require().Equal(newLock.Coins[0].Amount, tc.unlockAmount)

// check original synthetic lock
stakingDenom := keeper.StakingSyntheticDenom(lock.Coins[0].Denom, valAddr)
Copy link
Member

Choose a reason for hiding this comment

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

Can we also add check for total supply of stake denom to make sure that we have burnt the correct amount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added code to check the total supply of OSMO. Please let me know if you were thinking of something else

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.

Thank you @t4sk for the PR, i left few comments! 🐙

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.

Awesome! LGTM 🌮

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! 💯

@p0mvn
Copy link
Member

p0mvn commented Feb 2, 2023

Merging this since I can see 2 approvals. Thanks everyone!

@p0mvn p0mvn merged commit 55cb894 into osmosis-labs:main Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/superfluid V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants