Ensure that empty tranches are deleted #698
Conversation
|
Please attach a successful testrun 🙃 |
x/dex/keeper/cancel_limit_order.go
Outdated
| @@ -88,8 +88,8 @@ func (k Keeper) ExecuteCancelLimitOrder( | |||
| return sdk.Coin{}, sdk.Coin{}, nil, sdkerrors.Wrapf(types.ErrCancelEmptyLimitOrder, "%s", tranche.Key.TrancheKey) | |||
| } | |||
|
|
|||
| k.SaveTrancheUser(ctx, trancheUser) | |||
| k.SaveTranche(ctx, tranche) | |||
| k.SaveOrRemoveTrancheUser(ctx, trancheUser) | |||
There was a problem hiding this comment.
I mean, this is much more confusing for me now (it's not obvious at all when the method removes a tranche and when it saves a tranche)
Can't it be two separate methods?
There was a problem hiding this comment.
Like, i understand there is a pretty simple condition inside the method, but i think the code would be much easier to read if the condition would be located outside of the method
What do you think?
There was a problem hiding this comment.
I agree. I'm used to combined methods and functions like GetOrCreate and CreateOrUpdate, but SaveOrRemove are totally opposite operations, and the meaning is unclear for me. I'm still quite bad at the dex module, could you please explain the meaning to me? Even better: could you please add a comprehensive explanation to the comment of the method?
x/dex/keeper/cancel_limit_order.go
Outdated
| @@ -88,8 +88,8 @@ func (k Keeper) ExecuteCancelLimitOrder( | |||
| return sdk.Coin{}, sdk.Coin{}, nil, sdkerrors.Wrapf(types.ErrCancelEmptyLimitOrder, "%s", tranche.Key.TrancheKey) | |||
| } | |||
|
|
|||
| k.SaveTrancheUser(ctx, trancheUser) | |||
| k.SaveTranche(ctx, tranche) | |||
| k.SaveOrRemoveTrancheUser(ctx, trancheUser) | |||
There was a problem hiding this comment.
I agree. I'm used to combined methods and functions like GetOrCreate and CreateOrUpdate, but SaveOrRemove are totally opposite operations, and the meaning is unclear for me. I'm still quite bad at the dex module, could you please explain the meaning to me? Even better: could you please add a comprehensive explanation to the comment of the method?
x/dex/keeper/limit_order_tranche.go
Outdated
| func (k Keeper) SaveTranche(ctx sdk.Context, tranche *types.LimitOrderTranche) { | ||
| func (k Keeper) SaveOrMakeInactiveTranche(ctx sdk.Context, tranche *types.LimitOrderTranche) { | ||
| if tranche.HasTokenIn() { | ||
| k.SetLimitOrderTranche(ctx, tranche) | ||
| } else { | ||
| k.SetInactiveLimitOrderTranche(ctx, tranche) | ||
| k.SaveOrRemoveInactiveTranche(ctx, tranche) | ||
| k.RemoveLimitOrderTranche(ctx, tranche.Key) | ||
| ctx.EventManager().EmitEvents(types.GetEventsDecTotalOrders(tranche.Key.TradePairId)) | ||
| } |
There was a problem hiding this comment.
the method is called SaveOrMakeInactiveTranche, but inside we either
SetLimitOrderTranchewhich sound like theSave, orSaveOrRemoveInactiveTranchewhich doesn't sound likeMakeInactive
This is confusing. Could you please add comprehensive comments here and probably give these better names?
x/dex/keeper/place_limit_order.go
Outdated
|
|
||
| totalIn = totalIn.Add(amountLeft) | ||
| sharesIssued = amountLeft | ||
| } | ||
|
|
||
| k.SaveTrancheUser(ctx, trancheUser) | ||
| k.SaveOrRemoveTrancheUser(ctx, trancheUser) |
There was a problem hiding this comment.
we're placing a limit order here. can it be the case that we go to the first branch of the SaveOrRemoveTrancheUser that does RemoveLimitOrderTrancheUser in case if trancheUser.IsEmpty()?
| @@ -98,7 +98,7 @@ func (k Keeper) ExecuteWithdrawFilledLimitOrder( | |||
|
|
|||
| } | |||
|
|
|||
| k.SaveTrancheUser(ctx, trancheUser) | |||
| k.SaveOrRemoveTrancheUser(ctx, trancheUser) | |||
There was a problem hiding this comment.
we're withdrawing a filled limit order here. can it be the case that we go to the second branch of the SaveOrRemoveTrancheUser that does SetLimitOrderTrancheUser in case if !trancheUser.IsEmpty()?
There was a problem hiding this comment.
also could you please have a look at all the combined methods and
- check whether they are applied correctly or there can only be one of the conditions met;
- if combination is applied correctly in several places, which would imply that we need that method to avoid code duplication, cover the method and its usage with comments so it's clear for the reader how it works and why its called there
jcompagni10
force-pushed
the
fix/save_tranche_logic
branch
from
a4a6f9f to
865fba0
Compare
There are some cases when a tranche is canceled where is not deleted and instead is saved as an inactive tranche.
This doesn't pose any immediate problem, but in the long term would result in unnecessary state bloat.
this PR changes the k.saveTranche logic to ensure that empty tranches are fully deleted.