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

Return lock id from BeginForceUnlock #4059

Merged
merged 5 commits into from
Jan 23, 2023
Merged

Return lock id from BeginForceUnlock #4059

merged 5 commits into from
Jan 23, 2023

Conversation

t4sk
Copy link
Contributor

@t4sk t4sk commented Jan 18, 2023

What is the purpose of the change

Return lock id from BeginForceUnlock. This PR is 1st part of implementing partial withdraw from super fluid

#3830

Algorithm for SF partial withdraw is the following

1. SuperfluidUndelegate
2. Unbond lock for partial amount of the lock and return the new underlying lock that was split
3. Re-delegate remainder
4. Create synthetic unlocking lock for the new lock from step 2

This PR makes step 2 possible.

Brief Changelog

  • Return lock id from BeginForceUnlock

Testing and Verifying

(Please pick one of the following options)

  • This change is a trivial rework / code cleanup without any test coverage.

Documentation and Release Note

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

@ValarDragon ValarDragon added V:state/compatible/backport State machine compatible PR, should be backported A:backport/v14.x backport patches to v14.x branch labels Jan 18, 2023
ValarDragon
ValarDragon reviewed Jan 18, 2023
View reviewed changes
x/lockup/keeper/lock.go Outdated
Comment on lines 185 to 186
// the lock has a synthetic lock or not before unlocking.
func (k Keeper) BeginForceUnlock(ctx sdk.Context, lockID uint64, coins sdk.Coins) error {
func (k Keeper) BeginForceUnlock(ctx sdk.Context, lockID uint64, coins sdk.Coins) (uint64, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Lets add a comment for what is the uint64 being returned

Copy link
Member

Choose a reason for hiding this comment

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

|| named returned parameters!

ValarDragon
ValarDragon reviewed Jan 18, 2023
View reviewed changes
x/lockup/keeper/lock.go
}

// beginUnlock unlocks specified tokens from the given lock. Existing lock refs
// of not unlocking queue are deleted and new lock refs are then added.
// EndTime of the lock is set within this method.
// Coins provided as the parameter does not require to have all the tokens in the lock,
// as we allow partial unlockings of a lock.
func (k Keeper) beginUnlock(ctx sdk.Context, lock types.PeriodLock, coins sdk.Coins) error {
func (k Keeper) beginUnlock(ctx sdk.Context, lock types.PeriodLock, coins sdk.Coins) (uint64, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Lets add a comment for what is being returned. Needs to document what happens in both the case of a lock being split and not split.

@ValarDragon
Copy link
Member

ValarDragon commented Jan 18, 2023 •
edited
Loading

Thank you for the PR! Can you please add a test case for this?

p0mvn
p0mvn approved these changes Jan 19, 2023
View reviewed changes
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. Nice work

x/lockup/keeper/lock_test.go Show resolved Hide resolved
x/lockup/keeper/lock_test.go Outdated Show resolved Hide resolved
p0mvn
p0mvn approved these changes Jan 20, 2023
View reviewed changes
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.

Amazing. LGTM once minor comments are addressed

x/lockup/keeper/lock_test.go Outdated
{
name: "new lock id is returned if the lock was split",
coins: coins,
unlockCoins: sdk.Coins{sdk.NewInt64Coin("stake", 1)},
Copy link
Member

Choose a reason for hiding this comment

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

nit: while not a problem here, using the constructor is preferable because it does coin validation

Suggested change
unlockCoins: sdk.Coins{sdk.NewInt64Coin("stake", 1)},
unlockCoins: sdk.NewCoins(sdk.NewInt64Coin("stake", 1)),

x/lockup/keeper/lock_test.go Outdated
Comment on lines 72 to 73
for _, tc := range testCases {
suite.SetupTest()
Copy link
Member

Choose a reason for hiding this comment

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

For each test case, let's create a separate environment for it with suite.`Run(...). Similar to:

osmosis/x/gamm/keeper/pool_service_test.go

Lines 302 to 303 in 6286934

test := test
suite.Run(test.name, func() {

I know this is not done in most of the test cases in the file. However, this is the correct way that enables a separate test environment for each table-driven test case.

x/lockup/keeper/lock_test.go Outdated
locks, err = suite.App.LockupKeeper.GetPeriodLocks(suite.Ctx)
suite.Require().NoError(err)

for _, lock := range locks {
Copy link
Member

Choose a reason for hiding this comment

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

suggest checking that the locks length is not zero here. If we don't check and it happens so that 0 locks are returned, we would skip all the validations in the for loop completely

p0mvn
p0mvn approved these changes Jan 20, 2023
View reviewed changes
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. Thank you.

Will leave it open for now if anyone else would like to review it. If no updates, will merge tomorrow

@p0mvn
Copy link
Member

p0mvn commented Jan 23, 2023

Merging since no more eyes so far. Other reviewers can take a look later.

Thank you @t4sk

@p0mvn p0mvn merged commit 6ca0a56 into osmosis-labs:main Jan 23, 2023
@mergify mergify bot mentioned this pull request Jan 23, 2023
Merged
mergify bot pushed a commit that referenced this pull request Jan 23, 2023
@t4sk @mergify
Return lock id from BeginForceUnlock (#4059)
4911e7e 
* return lock id from BeginForceUnlock

* refactor and comment to BeginForceUnlock

* unit test BeginForceUnlock

* table driven test BeginForceUnlock

* update unit test for BeginForceUnlock

(cherry picked from commit 6ca0a56)
ValarDragon pushed a commit that referenced this pull request Jan 23, 2023
@mergify @t4sk
Return lock id from BeginForceUnlock (#4059) (#4078)
95d186d 
* return lock id from BeginForceUnlock

* refactor and comment to BeginForceUnlock

* unit test BeginForceUnlock

* table driven test BeginForceUnlock

* update unit test for BeginForceUnlock

(cherry picked from commit 6ca0a56)

Co-authored-by: t4sk <[email protected]>
@t4sk t4sk mentioned this pull request Jan 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattverse mattverse mattverse left review comments

@ValarDragon ValarDragon ValarDragon left review comments

@p0mvn p0mvn p0mvn approved these changes

Assignees
No one assigned
Labels
A:backport/v14.x backport patches to v14.x branch C:x/lockup C:x/superfluid V:state/compatible/backport State machine compatible PR, should be backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@t4sk @ValarDragon @p0mvn @mattverse