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(ecocredit/marketplace): nil expirations no longer pruned #1115

Merged
merged 8 commits into from
May 31, 2022
Next Next commit
fix: nil expirations no longer pruned
  • Loading branch information
technicallyty committed May 19, 2022
commit 4f596bc2cd0c8f70a3d98d6ce27d1fe16310b030
14 changes: 12 additions & 2 deletions x/ecocredit/server/marketplace/prune.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,26 @@ func (k Keeper) PruneSellOrders(ctx context.Context) error {
return err
}
defer it.Close()

expiredOrderIds := make([]uint64, 0)
for it.Next() {
sellOrder, err := it.Value()
if err != nil {
return err
}
if err = k.unescrowCredits(ctx, sellOrder.Seller, sellOrder.BatchId, sellOrder.Quantity); err != nil {
if sellOrder.Expiration != nil {
if err = k.unescrowCredits(ctx, sellOrder.Seller, sellOrder.BatchId, sellOrder.Quantity); err != nil {
return err
}
expiredOrderIds = append(expiredOrderIds, sellOrder.Id)
}
}
for _, id := range expiredOrderIds {
if err := k.stateStore.SellOrderTable().Delete(ctx, &marketplaceapi.SellOrder{Id: id}); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Actually, it looks like we can call this within the it.Next() loop rather than appending to expiredOrderIds. No need to create expiredOrderIds and append the expired orders and then loop through a second time.

Copy link
Contributor Author

@technicallyty technicallyty May 21, 2022

Choose a reason for hiding this comment

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

per https://github.com/cosmos/cosmos-sdk/blob/main/orm/model/ormtable/iterator.go#L15-L19 we should not mutate the table during iteration.

however, i came back to this and actually might have a better solution 😈 . so after talking with aaron, nil timestamps are actually treated/encoded as zero values, or timestamppb.Timestamp{Seconds:0, Nanos: 0}. so if we want the performance boost of DeleteRange, we could just delete everything starting at timestamppb.Timestamp{Seconds:0, Nanos: 1}.

the tests still hold up with this change 😄 . lmk if that sounds sane or not

return err
}
}
return k.stateStore.SellOrderTable().DeleteRange(ctx, fromKey, toKey)
return nil
}

// unescrowCredits moves `amount` of credits from the sellerAddr's escrowed balance, into their tradable balance.
Expand Down
38 changes: 38 additions & 0 deletions x/ecocredit/server/marketplace/prune_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,41 @@ func TestSell_Prune(t *testing.T) {
_, err = s.marketStore.SellOrderTable().Get(s.ctx, shouldBeValid)
assert.NilError(t, err)
}

// TestPrune_NilExpiration tests that sell orders with nil expirations are not deleted when PruneOrders is called.
func TestPrune_NilExpiration(t *testing.T) {
t.Parallel()
s := setupBase(t)
s.testSellSetup(batchDenom, ask.Denom, ask.Denom[1:], "C01", start, end, creditType)

blockTime, err := time.Parse("2006-01-02", "2020-01-01")
assert.NilError(t, err)
expired, err := time.Parse("2006-01-02", "2010-01-01")
assert.NilError(t, err)

msg := &marketplace.MsgSell{
Owner: s.addr.String(),
Orders: []*marketplace.MsgSell_Order{
{BatchDenom: batchDenom, Quantity: "5", AskPrice: &ask, Expiration: nil},
{BatchDenom: batchDenom, Quantity: "10", AskPrice: &ask, Expiration: &expired},
},
}
res, err := s.k.Sell(s.ctx, msg)
assert.NilError(t, err)

shouldExistOrder := res.SellOrderIds[0]
shouldNotExistOrder := res.SellOrderIds[1]

s.sdkCtx = s.sdkCtx.WithBlockTime(blockTime)
s.ctx = sdk.WrapSDKContext(s.sdkCtx)

err = s.k.PruneSellOrders(s.ctx)
assert.NilError(t, err)

order, err := s.marketStore.SellOrderTable().Get(s.ctx, shouldExistOrder)
assert.NilError(t, err)
assert.Equal(t, "5", order.Quantity)

order, err = s.marketStore.SellOrderTable().Get(s.ctx, shouldNotExistOrder)
assert.ErrorIs(t, err, ormerrors.NotFound)
}