From 51656d7e37072510d51ba409ded67f3084849ad1 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Fri, 14 Jul 2023 01:23:00 +0200 Subject: [PATCH] Make GetSuperfluidLockByLockId more cleary indicate if its found or not --- x/lockup/keeper/grpc_query.go | 14 +++++++++----- x/lockup/keeper/lock.go | 9 ++++++--- x/lockup/keeper/lock_test.go | 6 ++++-- x/lockup/keeper/msg_server.go | 3 ++- x/lockup/keeper/synthetic_lock.go | 20 ++++++++++++-------- x/lockup/keeper/synthetic_lock_test.go | 3 ++- x/superfluid/keeper/grpc_query.go | 2 +- x/superfluid/keeper/migrate.go | 3 ++- x/superfluid/keeper/stake.go | 6 ++++-- x/superfluid/types/expected_keepers.go | 2 +- x/valset-pref/types/expected_interfaces.go | 2 +- x/valset-pref/validator_set.go | 3 ++- 12 files changed, 46 insertions(+), 27 deletions(-) diff --git a/x/lockup/keeper/grpc_query.go b/x/lockup/keeper/grpc_query.go index ebd6e806232..db125806d3a 100644 --- a/x/lockup/keeper/grpc_query.go +++ b/x/lockup/keeper/grpc_query.go @@ -197,11 +197,15 @@ func (q Querier) SyntheticLockupsByLockupID(goCtx context.Context, req *types.Sy } ctx := sdk.UnwrapSDKContext(goCtx) - synthLock, err := q.Keeper.GetSyntheticLockupByUnderlyingLockId(ctx, req.LockId) + synthLock, found, err := q.Keeper.GetSyntheticLockupByUnderlyingLockId(ctx, req.LockId) if err != nil { return nil, err } - return &types.SyntheticLockupsByLockupIDResponse{SyntheticLocks: []types.SyntheticLock{synthLock}}, nil + synthlocks := []types.SyntheticLock{} + if found { + synthlocks = append(synthlocks, synthLock) + } + return &types.SyntheticLockupsByLockupIDResponse{SyntheticLocks: synthlocks}, nil } // SyntheticLockupByLockupID returns synthetic lockup by native lockup id. @@ -211,9 +215,9 @@ func (q Querier) SyntheticLockupByLockupID(goCtx context.Context, req *types.Syn } ctx := sdk.UnwrapSDKContext(goCtx) - synthLock, err := q.Keeper.GetSyntheticLockupByUnderlyingLockId(ctx, req.LockId) - if err != nil { - return nil, err + synthLock, found, err := q.Keeper.GetSyntheticLockupByUnderlyingLockId(ctx, req.LockId) + if err != nil || !found { + return &types.SyntheticLockupByLockupIDResponse{}, err } return &types.SyntheticLockupByLockupIDResponse{SyntheticLock: synthLock}, nil } diff --git a/x/lockup/keeper/lock.go b/x/lockup/keeper/lock.go index c9c1231093c..2b6859ce617 100644 --- a/x/lockup/keeper/lock.go +++ b/x/lockup/keeper/lock.go @@ -105,7 +105,8 @@ func (k Keeper) AddTokensToLockByID(ctx sdk.Context, lockID uint64, owner sdk.Ac return nil, err } - synthlock, err := k.GetSyntheticLockupByUnderlyingLockId(ctx, lock.ID) + // TODO: Handle found case in a better way, with state breaking update + synthlock, _, err := k.GetSyntheticLockupByUnderlyingLockId(ctx, lock.ID) if err != nil { return nil, err } @@ -382,7 +383,8 @@ func (k Keeper) ForceUnlock(ctx sdk.Context, lock types.PeriodLock) error { // 2) If lock is bonded, move it to unlocking // 3) Run logic to delete unlocking metadata, and send tokens to owner. - synthLock, err := k.GetSyntheticLockupByUnderlyingLockId(ctx, lock.ID) + // TODO: Use found instead of !synthLock.IsNil() later on. + synthLock, _, err := k.GetSyntheticLockupByUnderlyingLockId(ctx, lock.ID) if err != nil { return err } @@ -766,7 +768,8 @@ func (k Keeper) removeTokensFromLock(ctx sdk.Context, lock *types.PeriodLock, co } // increase synthetic lockup's accumulation store - synthLock, err := k.GetSyntheticLockupByUnderlyingLockId(ctx, lock.ID) + // TODO: In next state break, do err != nil || found == false + synthLock, _, err := k.GetSyntheticLockupByUnderlyingLockId(ctx, lock.ID) if err != nil { return err } diff --git a/x/lockup/keeper/lock_test.go b/x/lockup/keeper/lock_test.go index 2450d015909..ed1287a5d23 100644 --- a/x/lockup/keeper/lock_test.go +++ b/x/lockup/keeper/lock_test.go @@ -1042,8 +1042,9 @@ func (s *KeeperTestSuite) AddTokensToLockForSynth() { } // by GetSyntheticLockupByUnderlyingLockId for i := uint64(1); i <= 3; i++ { - synthlockByLockup, err := s.App.LockupKeeper.GetSyntheticLockupByUnderlyingLockId(s.Ctx, i) + synthlockByLockup, found, err := s.App.LockupKeeper.GetSyntheticLockupByUnderlyingLockId(s.Ctx, i) s.Require().NoError(err) + s.Require().True(found) s.Require().Equal(synthlockByLockup, synthlocks[(int(i)-1)*3+int(i)]) } @@ -1503,8 +1504,9 @@ func (s *KeeperTestSuite) TestForceUnlock() { // if it was superfluid delegated lock, // confirm that we don't have associated synth lock - synthLock, err := s.App.LockupKeeper.GetSyntheticLockupByUnderlyingLockId(s.Ctx, lock.ID) + synthLock, found, err := s.App.LockupKeeper.GetSyntheticLockupByUnderlyingLockId(s.Ctx, lock.ID) s.Require().NoError(err) + s.Require().False(found) s.Require().Equal((lockuptypes.SyntheticLock{}), synthLock) // check if lock is deleted by checking trying to get lock ID diff --git a/x/lockup/keeper/msg_server.go b/x/lockup/keeper/msg_server.go index b0d424a4e78..df17b8c19c1 100644 --- a/x/lockup/keeper/msg_server.go +++ b/x/lockup/keeper/msg_server.go @@ -208,7 +208,8 @@ func (server msgServer) ForceUnlock(goCtx context.Context, msg *types.MsgForceUn } // check that given lock is not superfluid staked - synthLock, err := server.keeper.GetSyntheticLockupByUnderlyingLockId(ctx, lock.ID) + // TODO: Next state break do found, instead !synthlock.IsNil() + synthLock, _, err := server.keeper.GetSyntheticLockupByUnderlyingLockId(ctx, lock.ID) if err != nil { return &types.MsgForceUnlockResponse{Success: false}, errorsmod.Wrap(sdkerrors.ErrInvalidRequest, err.Error()) } diff --git a/x/lockup/keeper/synthetic_lock.go b/x/lockup/keeper/synthetic_lock.go index 1e1020bb592..7bd1be44967 100644 --- a/x/lockup/keeper/synthetic_lock.go +++ b/x/lockup/keeper/synthetic_lock.go @@ -39,7 +39,10 @@ func (k Keeper) GetSyntheticLockup(ctx sdk.Context, lockID uint64, synthdenom st // 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) { +// Returns (syntheticLockup, found, error) +// intended behavior for most callers is to check: +// if !found || err != nil { handle_it } +func (k Keeper) GetSyntheticLockupByUnderlyingLockId(ctx sdk.Context, lockID uint64) (types.SyntheticLock, bool, error) { store := ctx.KVStore(k.storeKey) iterator := sdk.KVStorePrefixIterator(store, combineKeys(types.KeyPrefixSyntheticLockup, sdk.Uint64ToBigEndian(lockID))) defer iterator.Close() @@ -49,17 +52,17 @@ func (k Keeper) GetSyntheticLockupByUnderlyingLockId(ctx sdk.Context, lockID uin synthLock := types.SyntheticLock{} err := proto.Unmarshal(iterator.Value(), &synthLock) if err != nil { - return types.SyntheticLock{}, err + return types.SyntheticLock{}, true, err } synthLocks = append(synthLocks, synthLock) } if len(synthLocks) > 1 { - return types.SyntheticLock{}, fmt.Errorf("synthetic lockup with same lock id should not exist") + return types.SyntheticLock{}, true, fmt.Errorf("synthetic lockup with same lock id should not exist") } if len(synthLocks) == 0 { - return types.SyntheticLock{}, nil + return types.SyntheticLock{}, false, nil } - return synthLocks[0], nil + return synthLocks[0], true, nil } // GetAllSyntheticLockupsByAddr gets all the synthetic lockups from all the locks owned by the given address. @@ -67,11 +70,11 @@ func (k Keeper) GetAllSyntheticLockupsByAddr(ctx sdk.Context, owner sdk.AccAddre synthLocks := []types.SyntheticLock{} locks := k.GetAccountPeriodLocks(ctx, owner) for _, lock := range locks { - synthLock, err := k.GetSyntheticLockupByUnderlyingLockId(ctx, lock.ID) + synthLock, found, err := k.GetSyntheticLockupByUnderlyingLockId(ctx, lock.ID) if err != nil { panic(err) } - if synthLock.UnderlyingLockId != 0 { + if found { synthLocks = append(synthLocks, synthLock) } } @@ -110,7 +113,8 @@ func (k Keeper) CreateSyntheticLockup(ctx sdk.Context, lockID uint64, synthDenom // There is no relationship between unbonding and bonding synthetic lockup, it's managed separately // A separate accumulation store is incremented with the synth denom. - synthLock, err := k.GetSyntheticLockupByUnderlyingLockId(ctx, lockID) + // TODO: Next state break change !synthlock.IsNil -> found + synthLock, _, err := k.GetSyntheticLockupByUnderlyingLockId(ctx, lockID) if err != nil { return err } diff --git a/x/lockup/keeper/synthetic_lock_test.go b/x/lockup/keeper/synthetic_lock_test.go index 608b871e339..0b9bc4228e1 100644 --- a/x/lockup/keeper/synthetic_lock_test.go +++ b/x/lockup/keeper/synthetic_lock_test.go @@ -139,8 +139,9 @@ func (s *KeeperTestSuite) TestSyntheticLockupCreateGetDeleteAccumulation() { }) expectedSynthLock := *synthLock - actualSynthLock, err := s.App.LockupKeeper.GetSyntheticLockupByUnderlyingLockId(s.Ctx, 1) + actualSynthLock, found, err := s.App.LockupKeeper.GetSyntheticLockupByUnderlyingLockId(s.Ctx, 1) s.Require().NoError(err) + s.Require().True(found) s.Require().Equal(expectedSynthLock, actualSynthLock) allSynthLocks := s.App.LockupKeeper.GetAllSyntheticLockups(s.Ctx) diff --git a/x/superfluid/keeper/grpc_query.go b/x/superfluid/keeper/grpc_query.go index e35c5e4464c..f241ec35b3c 100644 --- a/x/superfluid/keeper/grpc_query.go +++ b/x/superfluid/keeper/grpc_query.go @@ -656,7 +656,7 @@ func (q Querier) filterConcentratedPositionLocks(ctx sdk.Context, positions []mo return nil, err } - syntheticLock, err := q.Keeper.lk.GetSyntheticLockupByUnderlyingLockId(ctx, lockId) + syntheticLock, _, err := q.Keeper.lk.GetSyntheticLockupByUnderlyingLockId(ctx, lockId) if err != nil { return nil, err } diff --git a/x/superfluid/keeper/migrate.go b/x/superfluid/keeper/migrate.go index e4857535161..47257b4bfa6 100644 --- a/x/superfluid/keeper/migrate.go +++ b/x/superfluid/keeper/migrate.go @@ -237,11 +237,12 @@ func (k Keeper) routeMigration(ctx sdk.Context, sender sdk.AccAddress, providedL lockId := uint64(providedLockId) - synthLockBeforeMigration, err = k.lk.GetSyntheticLockupByUnderlyingLockId(ctx, lockId) + synthLockBeforeMigration, _, err = k.lk.GetSyntheticLockupByUnderlyingLockId(ctx, lockId) if err != nil { return lockuptypes.SyntheticLock{}, Unsupported, err } + // TODO: Change to if !found if synthLockBeforeMigration == (lockuptypes.SyntheticLock{}) { migrationType = NonSuperfluid } else if strings.Contains(synthLockBeforeMigration.SynthDenom, "superbonding") { diff --git a/x/superfluid/keeper/stake.go b/x/superfluid/keeper/stake.go index 9f9fc3b92df..bd40b110a77 100644 --- a/x/superfluid/keeper/stake.go +++ b/x/superfluid/keeper/stake.go @@ -469,10 +469,11 @@ func (k Keeper) unbondLock(ctx sdk.Context, underlyingLockId uint64, sender stri if err != nil { return 0, err } - synthLock, err := k.lk.GetSyntheticLockupByUnderlyingLockId(ctx, underlyingLockId) + synthLock, _, err := k.lk.GetSyntheticLockupByUnderlyingLockId(ctx, underlyingLockId) if err != nil { return 0, err } + // TODO: Use !found if synthLock == (lockuptypes.SyntheticLock{}) { return 0, types.ErrNotSuperfluidUsedLockup } @@ -496,10 +497,11 @@ func (k Keeper) alreadySuperfluidStaking(ctx sdk.Context, lockID uint64) bool { return true } - synthLock, err := k.lk.GetSyntheticLockupByUnderlyingLockId(ctx, lockID) + synthLock, _, err := k.lk.GetSyntheticLockupByUnderlyingLockId(ctx, lockID) if err != nil { return false } + // TODO: return found return synthLock != (lockuptypes.SyntheticLock{}) } diff --git a/x/superfluid/types/expected_keepers.go b/x/superfluid/types/expected_keepers.go index f4481e83f9d..34106f4db0d 100644 --- a/x/superfluid/types/expected_keepers.go +++ b/x/superfluid/types/expected_keepers.go @@ -42,7 +42,7 @@ type LockupKeeper interface { GetAllSyntheticLockups(ctx sdk.Context) []lockuptypes.SyntheticLock CreateSyntheticLockup(ctx sdk.Context, lockID uint64, suffix string, unlockDuration time.Duration, isUnlocking bool) error DeleteSyntheticLockup(ctx sdk.Context, lockID uint64, suffix string) error - GetSyntheticLockupByUnderlyingLockId(ctx sdk.Context, lockID uint64) (lockuptypes.SyntheticLock, error) + GetSyntheticLockupByUnderlyingLockId(ctx sdk.Context, lockID uint64) (lockuptypes.SyntheticLock, bool, error) } type LockupMsgServer interface { diff --git a/x/valset-pref/types/expected_interfaces.go b/x/valset-pref/types/expected_interfaces.go index 16e93a5ead0..13069f8ed77 100644 --- a/x/valset-pref/types/expected_interfaces.go +++ b/x/valset-pref/types/expected_interfaces.go @@ -33,7 +33,7 @@ type DistributionKeeper interface { } type LockupKeeper interface { GetLockByID(ctx sdk.Context, lockID uint64) (*lockuptypes.PeriodLock, error) - GetSyntheticLockupByUnderlyingLockId(ctx sdk.Context, lockID uint64) (lockuptypes.SyntheticLock, error) + GetSyntheticLockupByUnderlyingLockId(ctx sdk.Context, lockID uint64) (lockuptypes.SyntheticLock, bool, error) ForceUnlock(ctx sdk.Context, lock lockuptypes.PeriodLock) error BeginUnlock(ctx sdk.Context, lockID uint64, coins sdk.Coins) (uint64, error) GetPeriodLocks(ctx sdk.Context) ([]lockuptypes.PeriodLock, error) diff --git a/x/valset-pref/validator_set.go b/x/valset-pref/validator_set.go index 791d31e35d8..74b0e45bd70 100644 --- a/x/valset-pref/validator_set.go +++ b/x/valset-pref/validator_set.go @@ -353,10 +353,11 @@ func (k Keeper) ForceUnlockBondedOsmo(ctx sdk.Context, lockID uint64, delegatorA } // Ensured the lock has no superfluid relation by checking that there are no synthetic locks - synthLocks, err := k.lockupKeeper.GetSyntheticLockupByUnderlyingLockId(ctx, lockID) + synthLocks, _, err := k.lockupKeeper.GetSyntheticLockupByUnderlyingLockId(ctx, lockID) if err != nil { return sdk.Coin{}, err } + // TODO: use found if synthLocks != (lockuptypes.SyntheticLock{}) { return sdk.Coin{}, fmt.Errorf("cannot use DelegateBondedTokens being used for superfluid.") }