fix(ecocredit/marketplace): nil expirations no longer pruned #1115
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1115 +/- ##
==========================================
- Coverage 69.78% 64.10% -5.68%
==========================================
Files 218 215 -3
Lines 22243 20357 -1886
==========================================
- Hits 15522 13050 -2472
- Misses 5391 6049 +658
+ Partials 1330 1258 -72
Flags with carried forward coverage won't be shown. Click here to find out more. |
| } | ||
| } | ||
| for _, id := range expiredOrderIds { | ||
| if err := k.stateStore.SellOrderTable().Delete(ctx, &marketplaceapi.SellOrder{Id: id}); err != nil { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
Seeing this I really think this should be fixed in the orm because otherwise it's unexpected behavior |
|
going to convert to draft here pending #1115 (comment), we might not need this |
Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>
…regen-ledger into ty/1114-prune_bug
|
merging here, can revert with #1136 when the ORM changes are integrated |
|
Hmm... why did project code coverage go down by more than 5%?
|
|
Looks like it went back up more than 5% with the latest commit... Not exactly sure what happened there. |
Description
Fixes a bug where sell orders with nil expirations were being pruned
Closes: #1114
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change