Process Dutch auction NFT metadata in the block processor #1015

jessepinho · 2024-04-30T20:59:15Z

As part of #942, I've made a massive branch with a ton of changes to the Dutch auction UI. To make the review process more manageable, I'm breaking the changes into smaller atomic PRs.

This PR modifies the block processor to identify Dutch auctions, and save their NFTs' metadata to the database when it encounters them. Future PRs will build on top of this.

jessepinho · 2024-04-30T21:06:57Z

packages/query/src/block-processor.ts

@@ -52,10 +56,16 @@ interface QueryClientProps {
  stakingTokenMetadata: Metadata;
 }

-const blankTxSource = new CommitmentSource({
+const BLANK_TX_SOURCE = new CommitmentSource({


Just a rename to make it clear that this is a never-changing constant

jessepinho · 2024-04-30T21:07:15Z

packages/query/src/block-processor.ts

  source: { case: 'transaction', value: { id: new Uint8Array() } },
 });

+const POSITION_STATES: PositionState[] = [


Sort of the same here — extracting this constant from inside of a function, where it was being defined on every function call despite never changing.

jessepinho · 2024-04-30T21:10:02Z

packages/query/src/block-processor.ts

@@ -361,45 +371,66 @@ export class BlockProcessor implements BlockProcessorInterface {
    return spentNullifiers;
  }

-  private async identifyLpNftPositions(txs: Transaction[]) {


You'll probably want to review the rest of this file with "Hide whitespace" turned on. In short, I:

created a new processTransactions method that iterates over each transaction, then each action of each transaction, and calls this.identifyAuctionNfts() and this.identifyLpNftPositions() on each action.

created a new identifyAuctioNfts() method that saves auction NFT metadata when it appears.

modified identifyLpNftPositions() to process individual actions, rather than processing an array of transactions.

this a great design choice as it's extensible to other metadata-related processing we'll need to do in the future!

jessepinho · 2024-04-30T21:15:25Z

packages/wasm/crate/src/metadata.rs

@@ -73,21 +83,32 @@ mod test_helpers {

    use super::*;

-    pub fn get_metadata_for(display_denom: &str) -> Metadata {
+    pub fn get_metadata_for(display_denom: &str, base_denom_is_display_denom: bool) -> Metadata {


This is just a test helper. Auction NFTs only have one denom unit, so the base denom is the same as the display denom. So I added this arg to this function so that I can generate the right type of metadata for auction NFTs as well.

smooth brain question: is it ever the case that a base denom is not the display denom?

Yeah — the display denom is the one you see in the UI (e.g., "penumbra") but the base denom is the smallest possible unit (e.g., "upenumbra"). Usually they're different, just like US dollars and cents are different, but auction NFTs apparently only have a single denom unit — which makes sense, since you'll never hold more than 1 NFT for a given auction.

packages/wasm/crate/src/planner.rs

packages/wasm/crate/src/tx.rs

erwanor · 2024-05-02T03:51:50Z

packages/wasm/crate/src/planner.rs

@@ -101,7 +101,7 @@ impl ActionList {
        for value in self.balance().provided() {
            self.change_outputs.insert(
                value.asset_id,
-                OutputPlan::new(&mut OsRng, value, change_address),
+                OutputPlan::new(&mut OsRng, value, change_address.clone()),


I think I saw something about removing Copy from Address, this would explain why it's moved all the sudden

Anyway, cloning is the right thing to do here. LGTM.

yeah was getting lot of these copy errors too without cloning

erwanor · 2024-05-02T13:57:27Z

packages/wasm/crate/src/metadata.rs

@@ -73,21 +83,32 @@ mod test_helpers {

    use super::*;

-    pub fn get_metadata_for(display_denom: &str) -> Metadata {
+    pub fn get_metadata_for(display_denom: &str, base_denom_is_display_denom: bool) -> Metadata {


smooth brain question: is it ever the case that a base denom is not the display denom?

erwanor · 2024-05-02T13:57:58Z

packages/query/src/block-processor.ts

+      const auctionId = getAuctionId(action.value.description);
+      const metadata = getAuctionNftMetadata(
+        auctionId,
+        // Always a sequence number of 0 when starting a Dutch auction


erwanor · 2024-05-02T13:58:08Z

packages/query/src/block-processor.ts

+    } else if (action.case === 'actionDutchAuctionEnd' && action.value.auctionId) {
+      const metadata = getAuctionNftMetadata(
+        action.value.auctionId,
+        // Always a sequence number of 1 when ending a Dutch auction


erwanor · 2024-05-02T14:00:41Z

packages/query/src/block-processor.ts

+    }
+    /**
+     * @todo Handle `actionDutchAuctionWithdraw`, and figure out how to
+     * determine the sequence number if there have been multiple withdrawals.


I think one way to do that is to structure the Action['action'] with a sequence number, rather than adding it ex-post in this method, unsure

Cool, thanks, added a link to this comment in the ticket (#1020)

TalDerei · 2024-05-02T14:31:16Z

packages/query/src/block-processor.ts

@@ -361,45 +371,66 @@ export class BlockProcessor implements BlockProcessorInterface {
    return spentNullifiers;
  }

-  private async identifyLpNftPositions(txs: Transaction[]) {


this a great design choice as it's extensible to other metadata-related processing we'll need to do in the future!

TalDerei · 2024-05-02T14:33:51Z

packages/query/src/block-processor.ts

@@ -261,7 +271,7 @@ export class BlockProcessor implements BlockProcessorInterface {
        // - detect LpNft position opens
        // - generate all possible position state metadata
        // - update idb
-        await this.identifyLpNftPositions(blockTx);


can we update the description above processTransactions(blockTx) to accurately describe this function? Can we also move these existing comments to identifyLpNftPositions() and add comments to identifyAuctionNfts().

Whoops, good catch — fixed.

TalDerei · 2024-05-02T14:37:48Z

packages/wasm/crate/Cargo.toml

-penumbra-stake         = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.73.0", package = "penumbra-stake", default-features = false }
-penumbra-tct           = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.73.0", package = "penumbra-tct" }
-penumbra-transaction   = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.73.0", package = "penumbra-transaction", default-features = false }
+# TODO: Use `tag` instead of `rev` once auctions land in a tagged release of


TalDerei · 2024-05-02T14:41:38Z

packages/wasm/crate/src/planner.rs

@@ -101,7 +101,7 @@ impl ActionList {
        for value in self.balance().provided() {
            self.change_outputs.insert(
                value.asset_id,
-                OutputPlan::new(&mut OsRng, value, change_address),
+                OutputPlan::new(&mut OsRng, value, change_address.clone()),


yeah was getting lot of these copy errors too without cloning

TalDerei · 2024-05-02T14:55:23Z

save their NFTs' metadata to the database

we're currently only saving the auction metadata to the ASSETS table in indexed-db, where the assetId is used as a key for the table.

can you create a separate issue in the tracking issue for that builds on this? ie. creating a auction state table which stores the auction id and note commitment?

Add an index-db table (https://github.dev/penumbra-zone/web/blob/c73a8e2cac816b44540003eabe3a06ce876127ea/packages/types/src/indexed-db.ts#L51-L52). Reference the view server PR for the sql construction. Make the AuctionId the key in the table.
Define methods for reading, writing, and updating data in the table (https://github.dev/penumbra-zone/web/blob/c73a8e2cac816b44540003eabe3a06ce876127ea/packages/storage/src/indexed-db/index.ts#L79-L80)

TalDerei · 2024-05-02T14:58:45Z

The functionality seems to be all here, LGTM!

jessepinho · 2024-05-02T21:08:15Z

@TalDerei

can you create a separate issue in the tracking issue for that builds on this?

Yeah, that's covered by #980. I didn't specifically fill in the details since I know I'll be working on it myself anyway (plus I'll be referencing the pairing doc you created).

jessepinho commented Apr 30, 2024

View reviewed changes

erwanor self-requested a review April 30, 2024 21:12

jessepinho commented Apr 30, 2024

View reviewed changes

Process Dutch auctions in the block processor

be1d9e5

jessepinho force-pushed the jessepinho/block-processor-auctions-web-942 branch from 698e062 to be1d9e5 Compare April 30, 2024 21:16

Fix Rust ownership issue

e713930

jessepinho commented Apr 30, 2024

View reviewed changes

packages/wasm/crate/src/planner.rs Show resolved Hide resolved

jessepinho changed the title ~~WIP: Process Dutch auctions in the block processor~~ WIP: Process Dutch auction NFT metadata in the block processor Apr 30, 2024

jessepinho added 2 commits April 30, 2024 14:37

Fix test file

458a537

Simplify a bit

3fd0181

TalDerei self-requested a review April 30, 2024 22:02

Fix mutable key issue

29d7497

jessepinho commented Apr 30, 2024

View reviewed changes

packages/wasm/crate/src/tx.rs Show resolved Hide resolved

jessepinho marked this pull request as ready for review April 30, 2024 22:38

jessepinho changed the title ~~WIP: Process Dutch auction NFT metadata in the block processor~~ Process Dutch auction NFT metadata in the block processor Apr 30, 2024

erwanor approved these changes May 2, 2024

View reviewed changes

TalDerei approved these changes May 2, 2024

View reviewed changes

jessepinho mentioned this pull request May 2, 2024

Create UI/planner support for withdrawing from an existing auction #1020

Closed

Fix comments

929e38f

jessepinho merged commit a9cb099 into main May 2, 2024
6 checks passed

jessepinho deleted the jessepinho/block-processor-auctions-web-942 branch May 2, 2024 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Process Dutch auction NFT metadata in the block processor #1015

Process Dutch auction NFT metadata in the block processor #1015

jessepinho commented Apr 30, 2024 •

edited

Loading

jessepinho Apr 30, 2024

jessepinho Apr 30, 2024

jessepinho Apr 30, 2024

TalDerei May 2, 2024

jessepinho Apr 30, 2024

erwanor May 2, 2024

jessepinho May 2, 2024

erwanor May 2, 2024

erwanor May 2, 2024

TalDerei May 2, 2024

erwanor May 2, 2024

erwanor May 2, 2024

erwanor May 2, 2024

erwanor May 2, 2024

jessepinho May 2, 2024

TalDerei May 2, 2024

TalDerei May 2, 2024

jessepinho May 2, 2024

TalDerei May 2, 2024

TalDerei May 2, 2024

TalDerei commented May 2, 2024 •

edited

Loading

TalDerei commented May 2, 2024

jessepinho commented May 2, 2024

Process Dutch auction NFT metadata in the block processor #1015

Process Dutch auction NFT metadata in the block processor #1015

Conversation

jessepinho commented Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalDerei commented May 2, 2024 • edited Loading

TalDerei commented May 2, 2024

jessepinho commented May 2, 2024

jessepinho commented Apr 30, 2024 •

edited

Loading

TalDerei commented May 2, 2024 •

edited

Loading