penumbra-zone / web

Apache License 2.0
10 stars 7 forks source link

fees: ux for multi-asset fees #1268

Closed TalDerei closed 1 week ago

TalDerei commented 2 weeks ago

References #1165 and https://github.com/penumbra-zone/web/issues/1265.

Multi-asset fees were implemented, allowing transactions to use an alternative token for fees if the user does not have UM tokens.

IndexedDB was modified to add an index on the asset ID of the note's value in the SPENDABLE_NOTES table. Indexes can improve query performance at the cost of additional disk space for storage.

Following the resolution of https://github.com/penumbra-zone/penumbra/issues/4306, the fee rendering in the transaction approval dialogue and transaction view page displayed values that appeared significantly higher than the actual fees being deducted in transactions. The values were divided by 10^6 to display the correct decimal fee amount.


before:

Screenshot 2024-06-09 at 10 29 40 PM

after:

Screenshot 2024-06-09 at 11 30 38 PM

alternative asset fees if the user doesn't have UM:

Screenshot 2024-06-09 at 11 44 37 PM

privacy warning for non-native alt fees

https://github.com/penumbra-zone/web/assets/70081547/33ad2c63-3a07-4edc-9816-9004c7a98bd2

changeset-bot[bot] commented 2 weeks ago

⚠️ No Changeset found

Latest commit: 5de82b8cd3c012a4a455305d89f34af125841cc9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Valentine1898 commented 2 weeks ago

Another thing that confuses me, you may have a small UM balance which may not be enough to pay the transaction fee. I assume that it is not enough to check just the balance, we need to make sure that this balance is more than necessary for the transaction

Valentine1898 commented 2 weeks ago
image

It also looks like a bug in your screenshot. Probably means UM instead of upenumbra

TalDerei commented 2 weeks ago

Another thing that confuses me, you may have a small UM balance which may not be enough to pay the transaction fee. I assume that it is not enough to check just the balance, we need to make sure that this balance is more than necessary for the transaction

If the user has an insufficient UM balance, the transaction will rightly fail. Rather than doing something more sophisticated, it's fine to let the transaction fail and prompt the user to fetch more UM. Multi-asset fees are intended primarily for IBC-in transfers where new users don't have UM, and they can perform a swap or price discovery via the auction mechanism with their native bridged assets.

There's really no way to know in advance if the balance will cover the transaction until the planner attempts to balance the transaction with the user's notes and perform fee estimations.

TalDerei commented 2 weeks ago

It also looks like a bug in your screenshot. Probably means UM instead of upenumbra

yes, this should be displaying UM instead of upenumbra following the exponent calculations. I will change the base denom to reflect this.

hdevalence commented 2 weeks ago

I haven't finished reviewing, but I want to pause first and make sure I'm understanding the approach here:

1. Are we replacing the staking token with any of the other tokens being used in the transaction, based on the assumption that the user has a balance in that token if it's being used in the transaction? If so, what if the user is already using up their entire balance of that token in the transaction, and thus has none left over for the fee? Shouldn't we instead be looking at the user's balances of other tokens to determine which token to use?

I think the simplest approach to get the desired behavior (users can deposit over IBC and immediately start using Penumbra) is to do the following:

  1. if the user has an UM balance, use that;
  2. otherwise, iterate through the list of allowed alt fee tokens and use the first one for which the user has a balance.

We already don't have a way to handle the "send max" use case in the UI, so there's no regression here. To fix that problem properly, we'd want to have a "MAX" button, and when it's set, the way the planner is invoked should change: rather than specifying outputs and filling in spends to balance, it should specify spends (spend every note available) and set the change address for the outputs to the destination address. This way fees are deducted from outputs rather than inputs. The broken behavior you're describing that would prevent that is already the case for the UM token, and most of the plumbing for a fix is in place. (This UX is not even possible on a chain like Ethereum, where there's no way to statically determine the exact fee ahead of time).

2. This may be my ignorance of how multi-asset fees work, but don't we need some guarantee of the value of another asset to be able to use it as a fee? i.e., don't we need to e.g., do a swap first into UM and then use it as the fee?

No, the initial multi-asset fee mechanism has independent prices for each token. The idea is that the prices in alternative tokens should be set higher (say 10x higher) so that the resource pricing is protected from price fluctuations. This also means that as training wheels, there's no dependence on the DEX prices being accurate, and no possibility that someone could manipulate prices on the DEX and turn that into a DoS attack on the chain. The intended design is that the fee component swaps the alternative fee token to the native token but this happens on the backend, internal to the fee component, and isn't implemented yet.

3. If we are truly using another asset type as the fee, we shouldn't hard-code `"UM"` in various places in the UI where fees are displayed, right?

Yes, that seems right. We might need to adjust the proto definitions for the TransactionView to this effect (to have a presupplied Metadata), or we could have the renderer do some janky searching through the rest of the transaction for a Metadata with matching asset ID.

TalDerei commented 2 weeks ago

I think the simplest approach...

update: the remaining action items are (1) supporting the "send max" functionality and (2) not hardcoding UM, which require proto-related modifications. To limit the scope, I suggest we fix this separately.