Handle includeInactive in ViewService#auctions
#1047
Conversation
jessepinho
force-pushed
the
jessepinho/auctions-rpc-include-inactive-web-980
branch
from
4f3b3de to
fa75d5c
Compare
jessepinho
force-pushed
the
jessepinho/auctions-rpc-include-inactive-web-980
branch
from
9a4e5f1 to
c59b57c
Compare
jessepinho
changed the title
includeInactive in ViewService#auctionsincludeInactive in ViewService#auctions
jessepinho
requested review from
grod220,
Valentine1898,
erwanor,
TalDerei and
turbocrime
| // Always a sequence number of 1 when ending a Dutch auction | ||
| const seqNum = 1n; | ||
|
|
||
| const metadata = getAuctionNftMetadata(action.value.auctionId, seqNum); |
There was a problem hiding this comment.
won't this request metadata for closed auctions since it's passing in seqNum of 1? Shouldn't it pass seqNum of 0 for active auctions, and only after save the metadata and update the auction sequence number? Or does it not matter here?
There was a problem hiding this comment.
won't this request metadata for closed auctions since it's passing in seqNum of 1?
Correct — although it's not exactly requesting the metadata, but rather generating it. (getAuctioNftMetadata() calls a WASM function under the hood that generates metadata for an asset that doesn't exist yet.)
Shouldn't it pass seqNum of 0 for active auctions, and only after save the metadata and update the auction sequence number?
Hm — we could request the NFT metadata for the open auction, and the modify its sequence number. But that feels a bit messy to me — if the algorithm for generating NFT metadata changes in the future such that closed-auction NFTs have different metadata than open-auction NFTs, this would break. Easier to just generate it from scratch for each auction state.
There was a problem hiding this comment.
yeah was thinking about that, I think generating the metadata is the right design choice here.
There was a problem hiding this comment.
requesting
wrong word choice here 😅
| @@ -33,6 +33,8 @@ const getBech32mAuctionId = ( | |||
| return captureGroups.auctionId; | |||
| }; | |||
|
|
|||
| const isInactive = (seqNum?: bigint) => (seqNum === undefined ? false : seqNum > 0n); | |||
There was a problem hiding this comment.
why would the sequence number ever be undefined?
There was a problem hiding this comment.
I made it optional since value (used on line 61 below) may have an undefined seqNum property if we haven't saved it yet for some reason.
* Support queryLatestState * Simplify * Remove unused method * Change method signature * Rework a bit * Remove unused import * Account for the fullnode returning a DutchAuction, not a DutchAuctionState * Fix type name check
* Build UI to end an auction * Fix layout issue * Tweak symbols * Reload auctions after scheduling or ending an auction * Save auction metadata when ending an auction * Rename helper
AuctionsRequests have anincludeInactiveproperty that was not yet supported by ourauctionsimplementation. This PR adds support for that property by A) keeping track of auctions' sequence numbers to determine whether they're active, and B) filtering out inactive auctions unlessincludeInactiveistrue.In this PR
seqNumcolumn to theAUCTIONStable (to mimic thestatecolumn in core's implementation)seqNumin the block processor when we encounter a Dutch auction-related action.includeInactivein theViewService#auctionsRPC method.Relates to #980 (but doesn't close it yet, since
queryLatestStateis not yet supported)