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

[Chore] Remove SF partial migration from balancer to CL #5874

Merged
merged 6 commits into from
Jul 26, 2023

Conversation

stackman27
Copy link
Contributor

@stackman27 stackman27 commented Jul 18, 2023

Closes: #XXX

What is the purpose of the change

Remove Partial Migration

Testing and Verifying

Remove Partial Migration Tests

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@stackman27 stackman27 requested review from czarcas7ic and p0mvn July 18, 2023 20:31
@stackman27 stackman27 added the V:state/breaking State machine breaking PR label Jul 18, 2023
@stackman27 stackman27 marked this pull request as ready for review July 18, 2023 20:33
@stackman27 stackman27 requested a review from mattverse as a code owner July 18, 2023 20:33
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.

luv2delete code

@p0mvn
Copy link
Member

p0mvn commented Jul 19, 2023

We might also want to remove this:

  • // Otherwise, we must split the lock and force unlock the partial shares to migrate.
    // This breaks and deletes associated synthetic locks.
    err = k.lk.PartialForceUnlock(ctx, *lock, sdk.NewCoins(sharesToMigrate))
    if err != nil {
    return sdk.Coins{}, err
    }

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

@czarcas7ic
Copy link
Member

@stackman27 Can you modify this

// Finish unlocking directly for locked or unlocking locks
if sharesToMigrate.Equal(gammSharesInLock) {
// If migrating the entire lock, force unlock.
// This breaks and deletes associated synthetic locks.
err = k.lk.ForceUnlock(ctx, *lock)
if err != nil {
return sdk.Coins{}, err
}
} else {
// Otherwise, we must split the lock and force unlock the partial shares to migrate.
// This breaks and deletes associated synthetic locks.
err = k.lk.PartialForceUnlock(ctx, *lock, sdk.NewCoins(sharesToMigrate))
if err != nil {
return sdk.Coins{}, err
}
}

To just check if they are equal? Other than that it looks good to me

@stackman27
Copy link
Contributor Author

@stackman27 Can you modify this

// Finish unlocking directly for locked or unlocking locks
if sharesToMigrate.Equal(gammSharesInLock) {
// If migrating the entire lock, force unlock.
// This breaks and deletes associated synthetic locks.
err = k.lk.ForceUnlock(ctx, *lock)
if err != nil {
return sdk.Coins{}, err
}
} else {
// Otherwise, we must split the lock and force unlock the partial shares to migrate.
// This breaks and deletes associated synthetic locks.
err = k.lk.PartialForceUnlock(ctx, *lock, sdk.NewCoins(sharesToMigrate))
if err != nil {
return sdk.Coins{}, err
}
}

To just check if they are equal? Other than that it looks good to me

added check around this! lmk what you think

x/superfluid/keeper/migrate.go Outdated Show resolved Hide resolved
@@ -84,8 +88,6 @@ func (k Keeper) migrateSuperfluidBondedBalancerToConcentrated(ctx sdk.Context,
return 0, sdk.Int{}, sdk.Int{}, sdk.Dec{}, 0, 0, 0, err
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 we should just take in the lock ID and not the sharesToMigrate, and instead just pull the shares directly in this method. This API only made sense when shares to migrate was chooseable which it no longer is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one of the thought process i had was since we only call the migration functions using RouteLockedBalancerToConcentratedMigration and MigrateUnlockedPositionFromBalancerToConcentrated does allow partial migration, i wanted to keep sharesToMigrate. For all the private migration functions like migrateSuperfluidBondedBalancerToConcentrated we can just default it to the entire lock coins which we do in validateSharesToMigrateUnlockAndExitBalancerPool

Copy link
Member

@czarcas7ic czarcas7ic Jul 26, 2023

Choose a reason for hiding this comment

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

Ah I see yeah, that makes sense, thanks!

@stackman27 stackman27 force-pushed the stack/remove-parial-sf-migration branch from c592b5f to 6829d30 Compare July 25, 2023 20:02
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.

Re-reviewed and its LGTM, merging, thanks!

@czarcas7ic czarcas7ic merged commit b65a184 into main Jul 26, 2023
@czarcas7ic czarcas7ic deleted the stack/remove-parial-sf-migration branch July 26, 2023 02:17
@github-actions github-actions bot mentioned this pull request Apr 1, 2024
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