Let users withdraw funds from auctions

[jessepinho]

screencap.mov

In this PR

• Refactor <DutchAuctionComponent /> to take buttonType and onClickButton props, rather than endButton and onClickEndButton props (since now the button may be a withdraw or end button).
• Create a new outstandingReserves property on the AUCTIONS table for storing outstanding reserves for an auction that has ended but not been withdrawn from.
• Handle withdraw actions in the planner.
• Extract a byte_array_to_base64 helper and use it throughout storage.rs.
• Extract a processActionDutchAuctionEnd() helper from the block processor.
  • Processing the action that ends Dutch auctions was getting complex and was entirely untested (like everything else in the block processor 😬 😬 ), so I extracted it to a helper and wrote tests.

Closes #1020

[jessepinho]

Is there a clean way to not have to repeat this error line below on line 376?

[turbocrime]

these are technically different errors - i would put 'could not find auction' here, and 'could not find reserves' is appropriate at the other.

consider also rewriting it in the pattern of other fns in this file, which seem a bit clearer.

[jessepinho]

This doesn't do anything yet, since core currently creates a stubbed withdraw view with an empty reserves array.

This is a known issue and will eventually be fixed. In the meantime, though, I'm writing the code here that will automatically populate the necessary metadata once core is updated.

[TalDerei]

flagging that I'll also be inspecting these views closely per #1040, possibly changing what we display in our TxP / TxV

[erwanor]

Yeah it's hardcoded for now, my sense is that we can get to it a bit later, not next week but the one after, let me know if you think this should be prioritized and i'll get to it sooner

[turbocrime]

this is in a couple places. i understand what it's doing here in idb,

web/packages/storage/src/indexed-db/index.ts

Lines 714 to 721 in 670034a

	const outstandingReserves = value.outstandingReserves
	? {
	input: value.outstandingReserves.input.toJson() as Jsonified<Value>,
	output: value.outstandingReserves.output.toJson() as Jsonified<Value>,
	}
	: 'outstandingReserves' in value
	? undefined
	: existingRecord?.outstandingReserves;

but i think it would be more appropriate to use some other mechanism, or use a value like null rather than exploiting the subtle distinction between an undefined value and a value that is undefined.

[jessepinho]

Hmmm... I tend to agree. But as I started to implement your suggestion of using null, I realized I'd either have to:

1. Make outstandingReserves required and type it as outstandingReserves?: { input: ..., output: ... } | null — meaning it can be undefined OR null, depending on whether outstandingReserves has previously been set and then cleared, OR
2. Pass outstandingReserves: null everywhere that upsertAuction is called, which sort of defeats the purpose of an upsert. (I'd designed the method so that you can only pass the properties you want to update.)

Alternatively, instead of using null or undefined, I could create a separate OUTSTANDING_AUCTION_RESERVES table that we add to / delete from at the appropriate times. I don't love it, since outstanding reserves are essentially a child of auction records and thus should probably be in the auctions table. But... 🤷🏻‍♂️

Thoughts? Suggestions?

[turbocrime]

Make outstandingReserves required and type it as outstandingReserves?: { input: ..., output: ... } | null — meaning it can be undefined OR null, depending on whether outstandingReserves has previously been set and then cleared

In storage, it's okay to use a simple optional on the stored type, as you'll never need to distinguish 'was never defined' from 'was previously defined but then cleared'

but your upsert parameter definitely needs three possible values, as you're taking three possible actions:

• preserve existing data
• write new data
• delete data

and these deserve clarity.

null and undefined are distinct values - loosely equal but not strictly equal, so something like
outstandingReserves === null can be a useful conditional.

notably some of this is converted to json - strictly, json has no undefined type, but does have null. i don't think null ever comes out of the bufbuild serializer, but it is valid input as part of a JsonValue or JsonObject. the de-serializer throws if you pass it a bare null but would successfully deserialize a null field as a missing field.

part of the issue also is you're essentially doing a deep object merge.

some options:

• never delete data on upsert, and require a separate delete
• break out outstandingReserves into multiple values to avoid deep comparison. the null/undefined problem remains, but is simpler
• use null value to clear outstandingReserves, and undefined to preserve it
• use null value to clear fields inside outstandingReserves, and undefined to preserve them
• use an optional object for outstandingReserves, optional fields inside outstandingReserves, and to clear the fields within outstaindgReserves use
  • a present but empty object
  • an object with null values
  • an object with defined empty object values
  • an object with defined new Value() values

[turbocrime]

i think keeping a table of active auctions is reasonable, as the table of all auctions may eventually be large.

[grod220]

I think we should be careful to not assign meaning to null vs undefined from an API perspective. This has been a long debated thing in javascript land and there isn't a plain-meaning consensus on it. I'm personally in the camp that including null in the language was a mistake (yet another win for Rust).

Think explicit indicators for update versus deletion would be nice. Perhaps something like:

type UpdateField<T> = { type: 'UPDATE'; value: T } | { type: 'DELETE' };

outstandingReserves?: UpdateField<{ input: Value; output: Value }>;

or perhaps a new unset field:

  upsertAuction<T extends DutchAuctionDescription>(
    auctionId: AuctionId,
    value: UpdateFields<T>,
    unset?: (keyof UpdateFields<T>)[],
  ): Promise<void>;

or even an explicit method:

  deleteAuctionFields<T extends DutchAuctionDescription>(
    auctionId: AuctionId,
    fields: (keyof UpdateFields<T>)[],
  ): Promise<void>;

[turbocrime]

the amount of discussion generated here makes me think we should not provide any merging upsert, but just expect the caller to construct the object they intend to store, and then clobber whatever's there

[jessepinho]

OK see this commit.

I like keeping upsertAuction as a merge function since there are three fields that need to be updated separately. So I broke out outstanding reserves into a separate table, and simply added/deleted records as the user ends/withdraws from an auction, respectively.

(I added a commit afterward to upgrade to the latest buf build/etc. packages, but you can safely ignore that.)

[jessepinho]

(cc @grod220 / @Valentine1898 / @turbocrime / @TalDerei )

[TalDerei]

what's the motivation here?

[jessepinho]

Eventually, I'll probably want to sort by e.g., most recent auction descending, or something.

[TalDerei]

I think helper functions for decluttering the block processor is great. For continuity, can we make processActionDutchAuctionSchedule and processActionDutchAuctionWithdraw helpers as well?

[jessepinho]

Good call — done!

[TalDerei]

can you remind me again we make this call in the planner to save the metadata, if we're already doing this in the block processor?

[jessepinho]

Huh, I actually didn't think of this. But now that you raise it, I think we'll still need to do it this way for these two use cases:

1. The user creates the auction via pcli, then views it via a Prax-connected dapp like minifront.

In this case, we need to save the metadata in the block processor, so it won't yet exist in IndexedDB.

2. The user creates the auction via a Prax-connected dapp like minifront.

In this case, we need to save the metadata during the planning request, or else the auction NFT will show up as "Unknown Asset" in the "Output" action in the txn approval dialog.

---

Fortunately, these are keyed by ID, so there won't be duplicate assets in the database.

[changeset-bot]

🦋 Changeset detected

Latest commit: 27cba71

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 10 packages

Name	Type
@penumbra-zone/perspective	Minor
@penumbra-zone/services	Minor
@penumbra-zone/storage	Minor
chrome-extension	Minor
minifront	Minor
@penumbra-zone/query	Minor
@penumbra-zone/wasm	Minor
@penumbra-zone/ui	Minor
@penumbra-zone/services-context	Patch
node-status	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[jessepinho]

Typo :)