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

lockup: Allow rebonding of unbonding tokens #4713

Closed
wants to merge 66 commits into from

Conversation

pysel
Copy link
Member

@pysel pysel commented Mar 23, 2023

Closes: #382

What is the purpose of the change

Adds a tx to allow users rebond an unbonding lock

There are 2 scenarios:

  1. Will create a new non-unbonding lock if the amount of coins is not nil and not equal to the amount of coins in the lock. The rebonded tokens will be subtracted from the original lock, otherwise it will be unchanged

  2. If the amount of tokens specified is nil or equal to the amount of coins in that lock, that means we will simply change the original lock's status from unlocking to non-unlocking (by setting the endTime to time.Time{})

Checklist

  • proto
  • logic
  • cli

Brief Changelog

osmosisd tx lockup rebond-tokens [ID]

Testing and Verifying

  • Unit test added

Documentation and Release Note

added to CHANGELOG.md

@pysel pysel added V:state/compatible/backport State machine compatible PR, should be backported A:backport/v15.x backport patches to v15.x branch labels Mar 23, 2023
@pysel
Copy link
Member Author

pysel commented Mar 24, 2023

Questions to reviewers:

  1. should we backport this to v15?
  2. wdyt about changing the name to relock-tokens?

@github-actions github-actions bot added the C:CLI label Mar 24, 2023
@pysel pysel marked this pull request as ready for review March 25, 2023 14:12
@pysel pysel changed the title lockup: allow rebonding of unbonding tokens lockup: Allow rebonding of unbonding tokens Mar 25, 2023
@pysel pysel requested a review from ValarDragon March 25, 2023 14:19
x/lockup/client/cli/tx.go Outdated Show resolved Hide resolved
x/lockup/keeper/lock.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
x/lockup/types/hooks.go Outdated Show resolved Hide resolved
x/lockup/types/msgs.go Show resolved Hide resolved
Comment on lines 726 to 747
func (k Keeper) RebondTokens(ctx sdk.Context, lockID uint64, owner sdk.AccAddress, coins sdk.Coins) error {
lock, err := k.GetLockByID(ctx, lockID)
if err != nil {
return err
}

if lock.Owner != owner.String() {
return fmt.Errorf("lock %d is not owned by %s", lockID, owner)
}

if !lock.IsUnlocking() {
return fmt.Errorf("lock %d is not unlocking, rebonding only possible in unlocking stage", lockID)
}

// If all checks pass, we can rebond the tokens
err = k.rebondTokens(ctx, owner, *lock, coins)
if err != nil {
return err
}

return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for having 2 methods? I think we only need one unexported method

Copy link
Member Author

Choose a reason for hiding this comment

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

was following the convention of this example. Also, it made sense to me to separate the function that asserts the provided parameters are valid from the function that actually executes the rebonding logic. wdyt, should we do it in one function?

Copy link
Member

Choose a reason for hiding this comment

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

I personally think that these can be grouped into one since we would not have further instances where we would have to call rebondTokens separately

@p0mvn p0mvn requested a review from czarcas7ic March 27, 2023 14:50
@pysel pysel requested a review from p0mvn March 31, 2023 03:05
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.

Nice! Thanks for this PR, left some comments, please take a look!

x/lockup/client/cli/tx.go Outdated Show resolved Hide resolved
x/lockup/client/cli/tx.go Outdated Show resolved Hide resolved
proto/osmosis/lockup/tx.proto Outdated Show resolved Hide resolved
x/lockup/keeper/lock.go Outdated Show resolved Hide resolved
Comment on lines 726 to 747
func (k Keeper) RebondTokens(ctx sdk.Context, lockID uint64, owner sdk.AccAddress, coins sdk.Coins) error {
lock, err := k.GetLockByID(ctx, lockID)
if err != nil {
return err
}

if lock.Owner != owner.String() {
return fmt.Errorf("lock %d is not owned by %s", lockID, owner)
}

if !lock.IsUnlocking() {
return fmt.Errorf("lock %d is not unlocking, rebonding only possible in unlocking stage", lockID)
}

// If all checks pass, we can rebond the tokens
err = k.rebondTokens(ctx, owner, *lock, coins)
if err != nil {
return err
}

return 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 personally think that these can be grouped into one since we would not have further instances where we would have to call rebondTokens separately

x/lockup/keeper/lock.go Outdated Show resolved Hide resolved
x/lockup/keeper/lock.go Outdated Show resolved Hide resolved
}

// Remove original lock's refs from state
err = k.deleteLockRefs(ctx, types.KeyPrefixUnlocking, lock)
Copy link
Member

Choose a reason for hiding this comment

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

Question: Do we not need to use the following?

Suggested change
err = k.deleteLockRefs(ctx, types.KeyPrefixUnlocking, lock)
err = k.deleteLockRefs(ctx, unlockingPrefix(lock.IsUnlocking()), lock)

Copy link
Member Author

Choose a reason for hiding this comment

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

not really, because we are already assured at this point that the lock is unlocking, but we can still add that, but currently there will be no purpose to that

x/lockup/keeper/lock_test.go Outdated Show resolved Hide resolved
x/lockup/keeper/lock_test.go Outdated Show resolved Hide resolved
@pysel pysel requested a review from stackman27 as a code owner April 11, 2023 01:55
@mattverse mattverse self-assigned this Apr 19, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2023

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label May 4, 2023
@czarcas7ic
Copy link
Member

I'm pretty sure this can't be backported as it is not state compatible

Copy link
Contributor

@nicolaslara nicolaslara left a comment

Choose a reason for hiding this comment

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

This looks good to me. I think we need tests for the splitting locks case (added a comment about it in the code). Once those are in I'm happy to merge it

x/lockup/keeper/lock.go Outdated Show resolved Hide resolved
Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

Nice work! @AlpinYukseloglu is going to give a final blessing on this later but looks good for merge to me!

Copy link
Contributor

@AlpinYukseloglu AlpinYukseloglu 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 this PR! I reviewed the changes quite thoroughly and the logic looks sound, but given how this PR touches critical parts of our codebase and seems quite sparsely tested, we've opted to push this to v21.

I've included a list of the classes of tests that I believe are important to include in code comments. I was originally going to commit these directly to the PR, but the scope was large enough to the point where this was blocking v20 release, so we opted to simply include the list here and aim to get this change in for v21.


// a sanity check to make sure the lock is in the correct state
if tc.unlock {
suite.Require().True(initialLock.IsUnlocking())
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be able to add an additional set of test cases (or extend these ones by adding a new test field for time elapsed) that covers cases where time has elapsed since the lock started unlocking? Specifically, it is important we test behavior around when:

  • the lock is partially finished unlocking (should function same as current tests)
  • the lock is exactly finished unlocking (should not allow rebonding, as it should no longer be unlocking)
  • the lock is past the unlocking period (should fail in all cases)

It would also be great to have cases covering behavior where BeginUnlock is run on part (not all) of lockedCoins, as this triggers a different logic branch that uses SplitLock that is currently untested.

Finally, we should also have test cases that include the creation of other locks and assert that they are unchanged.

Copy link
Member Author

Choose a reason for hiding this comment

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

hey @AlpinYukseloglu! Those are great points, thank you for your review!

Note: I changed the logic of tests to make it as readable as possible. Now, in a test case struct I include so called setupFunctions that are being ran prior to testing. These include: creation of initial lock, unlocking the lock, creating synthetic lockup, etc.

I addressed your comments in latest commits, here is a breakdown of everything for an easier review process:

  1. Partially finished unlocking lock
  2. Exactly finished unlocking lock
  3. Past unlocking period
  4. BeginUnlock on some fraction of locked coins
  5. Assertion that other locks are unchanged

Though, I have one concern about exactly unlocked lock (2). (2) works because I run EndBlocker in the test prior to rebonding a lock. Because of that, it automatically removes the lock if the current block time is exactly the endTime of a lock (matured).

However, in real scenario, when transactions of a block with block time equal to endTime of an unlocking lock are being executed, if someone tries to rebond this lock, they will be able to do so, since it was not yet removed from the store. I am a little confused as to why in case (2), we should fail? Since unlocking of maturing locks happens in EndBlocker, I do not see the reason for forbidding case (2) from passing

Copy link
Member Author

Choose a reason for hiding this comment

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

tagging for re-review of the new test

cc: @mattverse @p0mvn

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding on case (2): basically, the test works right now by simulating an unreal scenario - by calling endBlocker and then running rebonding logic both with the same blocktime is essentially equivalent to a scenario, when two consecutive blocks have the same block time

Copy link
Member

Choose a reason for hiding this comment

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

I think case 2 correct way would be being able to rebond if endblocker has not been called yet

defaultDuration := time.Second

// autoUnlockingStartHeight is a height at which EndBlocker starts automatically unlocking matured locks
autoUnlockingStartHeight := int64(7)
Copy link
Member Author

Choose a reason for hiding this comment

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

note: auto withdrawing of tokens does not begin prior to height 7

suite.Require().NoError(err)

// calling end blocker to automatically remove matured locks (no-op if setup functions did not modify blocktime)
lockup.EndBlocker(suite.Ctx, *suite.App.LockupKeeper)
Copy link
Member Author

Choose a reason for hiding this comment

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

note: this is needed for test cases that assert rebonding of unlocking lock at some stage of it's unlocking period (ex: half of unlocking time has elapsed, exactly the unlocking time has elapsed, unlocking time has elapsed in the past)

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label Oct 31, 2023
@pysel pysel removed the Stale label Oct 31, 2023
@pysel
Copy link
Member Author

pysel commented Oct 31, 2023

keeping alive

@ValarDragon
Copy link
Member

ValarDragon commented Nov 6, 2023

Thank you so much for the huge amount of work that went into this 🙏 , really appreciate it.

Unfortunately don't think it makes sense to merge given that its a breaking change that will likely be unused, and its better to have less code to maintain. Really sorry, this should have been cut off much earlier.

Would still love to give you a bounty for the work on this

@ValarDragon ValarDragon closed this Nov 6, 2023
@pysel
Copy link
Member Author

pysel commented Nov 6, 2023

hey @ValarDragon ! Thank you for letting me know, it is totally fine and I understand it! I will reach out to your @osmosis.team email, and thank you again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Allow "rebonding" during "unbonding" period
8 participants