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

fix: expect single synthetic lock per native lock ID #5265

Merged
merged 10 commits into from
May 26, 2023

Conversation

czarcas7ic
Copy link
Member

@czarcas7ic czarcas7ic commented May 22, 2023

Closes: #5246

What is the purpose of the change

After doing some investigation, it turns out that we should never have multiple synthetic locks be assigned to a single lock ID. Despite this, some of the functions (such as GetAllSyntheticLockupsByLockup) implied that this was possible by expecting an array of synthetic locks as the return value.

After feeling confident that my understanding of synthetic locks to underlying locks was correct, I removed this from functions and tests to prevent others from being confused in the future (which also indirectly solves the issue I made for migration).

Testing and Verifying

  • Did a state export of mainnet data and confirmed no locks currently point to multiple synthetic locks
  • Modified tests in synthetic_lock_test and migrate_test

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

Comment on lines -144 to -157
// DeleteAllSyntheticLocks iterates over given array of synthetic locks and deletes all individual synthetic locks.
func (k Keeper) DeleteAllSyntheticLocks(ctx sdk.Context, lock types.PeriodLock, synthLocks []types.SyntheticLock) error {
if len(synthLocks) == 0 {
return nil
}

for _, synthLock := range synthLocks {
err := k.DeleteSyntheticLockup(ctx, lock.ID, synthLock.SynthDenom)
if err != nil {
return err
}
}
return nil
}
Copy link
Member Author

Choose a reason for hiding this comment

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

No longer needed. We use DeleteSyntheticLockup directly now since we never expect multiple synthLocks anymore

@czarcas7ic czarcas7ic requested a review from p0mvn May 23, 2023 00:33
@czarcas7ic czarcas7ic marked this pull request as ready for review May 23, 2023 00:35
@czarcas7ic czarcas7ic changed the title fix: single synth lock per lock ID fix: expect single synthetic lock per native lock ID May 23, 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.

LGTM, I would prefer creating and using a synthLock.IsNil() method or so, but this can be handled in a separate issue if you agree!

proto/osmosis/lockup/query.proto Show resolved Hide resolved
// Error is returned if:
// - there are more than one synthetic lockup objects with the same underlying lock ID.
// - there is no synthetic lockup object with the given underlying lock ID.
func (k Keeper) GetSyntheticLockupByUnderlyingLockId(ctx sdk.Context, lockID uint64) (types.SyntheticLock, error) {
store := ctx.KVStore(k.storeKey)
iterator := sdk.KVStorePrefixIterator(store, combineKeys(types.KeyPrefixSyntheticLockup, sdk.Uint64ToBigEndian(lockID)))
Copy link
Member

Choose a reason for hiding this comment

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

If we are directly using key -value pair I think we should implement this without using iterators

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed iterator here e0ecfea

Copy link
Member Author

Choose a reason for hiding this comment

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

After looking into this, I don't think we can do this without a large refactor / state migration. We are currently keying by lockID and synthDenom, which is why we need to use the iterator. We absolutely do not need the synthDenom in the key, but if we removed it, we would need to do a state migration of all currently existing state entires.

I am going to revert back to the iterator method, but will wait for an ack on this before merging @mattverse

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

GetCmdSyntheticLockupsByLockupID(),
GetCmdSyntheticLockupByLockupID(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggest avoiding breaking CLI and instead adding a new CLI

Copy link
Member Author

Choose a reason for hiding this comment

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

Added back the old CLI cmd and added deprecated in the description 7bd3955

x/lockup/keeper/msg_server.go Outdated Show resolved Hide resolved
@p0mvn
Copy link
Member

p0mvn commented May 26, 2023

Merging since 2 ACKs and all feedback seems addressed

@p0mvn p0mvn merged commit b2aaef4 into main May 26, 2023
@p0mvn p0mvn deleted the adam/expect-single-synth-lock-to-lock branch May 26, 2023 02:49
pysel pushed a commit that referenced this pull request Jun 6, 2023
* single synth lock per lock ID

* add changelog entry

* deprecate SyntheticLockupsByLockupID

* small nits

* add deprecated to underlying messages

* remove iterator

* add IsNil check

* add back iterator

* add back old CLI command
@github-actions github-actions bot mentioned this pull request Mar 1, 2024
@github-actions github-actions bot mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CL: multiple synth locks for single lock
3 participants