Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fees: ux for multi-asset fees #1268

Merged
merged 29 commits into from
Jun 17, 2024
Merged

fees: ux for multi-asset fees #1268

merged 29 commits into from
Jun 17, 2024

Conversation

TalDerei
Copy link
Contributor

@TalDerei TalDerei commented Jun 9, 2024

References #1165 and #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 penumbra-zone/penumbra#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

Screen.Recording.2024-06-12.at.11.47.22.PM.mov

@TalDerei TalDerei self-assigned this Jun 9, 2024
Copy link

changeset-bot bot commented Jun 9, 2024

⚠️ No Changeset found

Latest commit: 5de82b8

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

@TalDerei TalDerei marked this pull request as draft June 9, 2024 09:43
@TalDerei TalDerei requested a review from a team June 10, 2024 08:30
@TalDerei TalDerei marked this pull request as ready for review June 10, 2024 08:30
@TalDerei TalDerei requested a review from hdevalence June 10, 2024 08:31
Copy link
Contributor

@Valentine1898 Valentine1898 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the comments have to do with the fact that we don't use registry in many places

packages/wasm/src/planner.ts Show resolved Hide resolved
packages/ui/components/ui/tx/view/transaction.tsx Outdated Show resolved Hide resolved
packages/types/src/indexed-db.ts Outdated Show resolved Hide resolved
packages/storage/src/indexed-db/index.ts Outdated Show resolved Hide resolved
packages/storage/src/indexed-db/index.ts Outdated Show resolved Hide resolved
packages/services/src/view-service/fees.ts Outdated Show resolved Hide resolved
@Valentine1898
Copy link
Contributor

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
Copy link
Contributor

Valentine1898 commented Jun 10, 2024

image

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

@TalDerei TalDerei requested a review from a team June 10, 2024 20:26
@TalDerei TalDerei changed the title fees: ux for multi-fees fees: ux for multi-asset fees Jun 10, 2024
@TalDerei
Copy link
Contributor Author

TalDerei commented Jun 11, 2024

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
Copy link
Contributor Author

TalDerei commented Jun 11, 2024

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.

Copy link
Contributor

@jessepinho jessepinho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
  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?
  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?

packages/services/src/test-utils.ts Outdated Show resolved Hide resolved
packages/services/src/view-service/fees.ts Outdated Show resolved Hide resolved
pnpm-lock.yaml Outdated Show resolved Hide resolved
packages/storage/src/indexed-db/index.ts Outdated Show resolved Hide resolved
packages/storage/src/indexed-db/index.ts Outdated Show resolved Hide resolved
packages/storage/src/indexed-db/index.ts Outdated Show resolved Hide resolved
packages/storage/src/indexed-db/index.ts Outdated Show resolved Hide resolved
packages/ui/components/ui/tx/view/transaction.tsx Outdated Show resolved Hide resolved
@hdevalence
Copy link
Member

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.

Copy link
Contributor

@Valentine1898 Valentine1898 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some ui tests are broken, need to fix them

apps/minifront/src/components/send/helpers.ts Outdated Show resolved Hide resolved
apps/minifront/src/components/send/helpers.ts Outdated Show resolved Hide resolved
apps/minifront/src/components/send/send-form/index.tsx Outdated Show resolved Hide resolved
apps/minifront/src/components/send/send-form/index.tsx Outdated Show resolved Hide resolved
packages/services/src/view-service/fees.ts Outdated Show resolved Hide resolved
packages/storage/src/indexed-db/index.ts Show resolved Hide resolved
packages/storage/src/indexed-db/index.ts Outdated Show resolved Hide resolved
packages/ui/components/ui/tx/view/transaction.tsx Outdated Show resolved Hide resolved
packages/ui/components/ui/tx/view/transaction.tsx Outdated Show resolved Hide resolved
Comment on lines +111 to +119
{showNonNativeFeeWarning && (
<div className='rounded border border-yellow-500 bg-gray-800 p-4 text-yellow-500'>
<strong>Privacy Warning:</strong>
<span className='block'>
Using non-native tokens for transaction fees may pose a privacy risk. It is recommended
to use the native token (UM) for better privacy and security.
</span>
</div>
)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it makes sense to make this a separate component to avoid duplication

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will cleanup the remaining suggestions in separately

packages/services/src/view-service/fees.ts Show resolved Hide resolved
packages/storage/src/indexed-db/index.ts Show resolved Hide resolved
@TalDerei TalDerei requested a review from Valentine1898 June 17, 2024 17:36
Copy link
Contributor

@Valentine1898 Valentine1898 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TalDerei Great work!

@TalDerei TalDerei merged commit 29fc9f7 into main Jun 17, 2024
6 checks passed
@TalDerei TalDerei deleted the ux-multi-fees branch June 17, 2024 18:46
@TalDerei TalDerei restored the ux-multi-fees branch July 2, 2024 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants