fee: v0 support for multi-asset fees #4320
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
references #4313 |
hdevalence
force-pushed
the
multi-asset-fees
branch
from
90c50ac to
b342ec1
Compare
hdevalence
added
consensus-breaking
hdevalence
commented
May 6, 2024
Comment on lines
+21
to
+36
| // When we implement dynamic gas pricing, we will want | ||
| // to read the prices we computed. But until then, we need to | ||
| // read these from the _fee params_ instead, since those are | ||
| // the values that will get updated by governance. | ||
| let params = self.get_fee_params().await?; | ||
| Ok(params.fixed_gas_prices) | ||
| } | ||
|
|
||
| /// Gets the current gas prices for alternative fee tokens. | ||
| async fn get_alt_gas_prices(&self) -> Result<Vec<GasPrices>> { | ||
| // When we implement dynamic gas pricing, we will want | ||
| // to read the prices we computed. But until then, we need to | ||
| // read these from the _fee params_ instead, since those are | ||
| // the values that will get updated by governance. | ||
| let params = self.get_fee_params().await?; | ||
| Ok(params.fixed_alt_gas_prices) |
There was a problem hiding this comment.
This fixes a critical bug in the original fee component logic, which would prevent fees from ever being changed post-genesis.
erwanor
approved these changes
May 6, 2024
| ); | ||
|
|
||
| // We split the check to provide granular error messages. | ||
| let transaction_base_fee = current_gas_prices.fee(&transaction.gas_cost()); | ||
|
|
||
| ensure!( |
There was a problem hiding this comment.
ACK, we:
- Extract the gas price vector based on the user supplied asset id
- Compute the base fee relative to those prices and the transaction gas cost
| user_supplied_fee.amount() >= transaction_base_fee.amount(), | ||
| "fee must be greater than or equal to the transaction base price (supplied: {}, base: {})", | ||
| user_supplied_fee.amount(), | ||
| transaction_base_fee.amount(), | ||
| ); |
There was a problem hiding this comment.
ACK, this work because both the user supplied base fee and the transaction's are computed based on the same gas prices.
| @@ -23,8 +23,6 @@ impl Component for Fee { | |||
| match app_state { | |||
| Some(genesis) => { | |||
| state.put_fee_params(genesis.fee_params.clone()); | |||
| // Put the initial gas prices | |||
| state.put_gas_prices(genesis.fee_params.fixed_gas_prices); | |||
There was a problem hiding this comment.
ACK, the gas prices are part of the fee parameters so no need to write them separately
This commit implements a v0 of multi-asset fees (on the protocol side).
Multi-asset fees are important because they allows users who send tokens into
Penumbra over IBC to start using the chain immediately.
It does this by extending the `GasPrices` struct to include the asset ID of the
fee token the prices are for. This allows natural handling of multi-asset fees
by just having multiple sets of `GasPrices`.
In the past, we've laid the groundwork for this (by allowing other assets in
the `Fee` structure, even if it defaults to the staking token) but haven't
prioritized it, due to concerns about some of the details of the
implementation, particularly:
A: How does the chain know that the prices of non-native fee tokens are correct?
B: What happens to the non-native fee tokens?
C: What are the privacy implications for users paying with non-native fee tokens?
This design punts on (A) by only allowing non-native fee tokens with prices
explicitly specified using governance. In the future, Penumbra could own DEX as
a price oracle, but this would be better to do after it's live and we have real
data about it, rather than worrying about edge cases now (e.g., if someone can
temporarily manipulate a price for their own trades, that's not a big deal in
itself, but hooking it to the fee mechanism used for resource usage could make
it one).
The approach to (C) is that these explicitly specified prices should be
substantially higher (10x) than those of the native token. This means that
regardless of price variability, it should remain the case that the native fee
token is cheaper, so that the majority of users will use the same fee token and
their transactions will not be distinguishable. On the other hand, letting it
be _possible_ to use non native fee tokens means that users who send IBC tokens
into Penumbra can access the protocol, and even if one of their first acts is
to swap some de minimis amount of the native token, they can do that within the
system.
This implementation currently does not properly handle (B), in that it silently
burns all fees, regardless of token. It is not appropriate to burn any tokens
other than the native token, whose supply is managed by the chain. Instead, the
transaction-level fee check should move into `check_and_execute`, and call a
method on the fee component that records the fee amount in the object store. At
the end of the block, all non-native fees should be swapped into the native
token and then burned. Emitted events should record:
- an `EventFee` for each transaction, with the transaction's gas cost, the gas
prices used to compute the base fee, the base fee, and the actual fee;
- an `EventFeeBurn` at the end of the block, with the total amount of native
fees burned as well as, for each alt fee token used to pay fees in that
block, how much of the alt token was paid and how much of the native token it
was traded for.
Client support is still mostly missing, though I have manually tested this on a
local devnet, by
1. Creating the devnet
2. Executing a parameter change proposal to set `dexParams.fixedAltGasPrices` to `"[{\"assetId\":{\"altBaseDenom\":\"ugm\"},\"blockSpacePrice\":\"80000\",\"compactBlockSpacePrice\":\"80000\",\"verificationPrice\":\"80000\",\"executionPrice\":\"80000\"}]"`
3. Hardcoding at the top of `TxCmd::exec` a change to the `gas_prices` to add the `gm` asset ID and multiplying all the prices by 10.
To properly support clients, we need to
- Record the `gas_prices` data for alt fee tokens that is newly included in the
`CompactBlock` in the view server's storage (probably in a new `gas_prices`
table that maps a fee token asset ID to the latest price);
- Use that data to populate the view server's gas prices RPC response (instead
of the current `Vec::new` stub that reports any alt fee tokens as
non-existing);
- Change the `pcli tx` command so that the existing `--fee-tier` and a new
`--fee-token` parameters move to `pcli tx`, and apply to all subcommands
(i.e., `pcli tx --fee-tier ... --fee-token gm send ...`) so that `TxCmd::exec`
can pull the right `GasPrices` out of the RPC response.
hdevalence
force-pushed
the
multi-asset-fees
branch
from
b342ec1 to
805e2fd
Compare
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes
This commit implements a v0 of multi-asset fees (on the protocol side).
Multi-asset fees are important because they allows users who send tokens into
Penumbra over IBC to start using the chain immediately.
It does this by extending the
GasPricesstruct to include the asset ID of thefee token the prices are for. This allows natural handling of multi-asset fees
by just having multiple sets of
GasPrices.In the past, we've laid the groundwork for this (by allowing other assets in
the
Feestructure, even if it defaults to the staking token) but haven'tprioritized it, due to concerns about some of the details of the
implementation, particularly:
A: How does the chain know that the prices of non-native fee tokens are correct?
B: What happens to the non-native fee tokens?
C: What are the privacy implications for users paying with non-native fee tokens?
This design punts on (A) by only allowing non-native fee tokens with prices
explicitly specified using governance. In the future, Penumbra could own DEX as
a price oracle, but this would be better to do after it's live and we have real
data about it, rather than worrying about edge cases now (e.g., if someone can
temporarily manipulate a price for their own trades, that's not a big deal in
itself, but hooking it to the fee mechanism used for resource usage could make
it one).
The approach to (C) is that these explicitly specified prices should be
substantially higher (10x) than those of the native token. This means that
regardless of price variability, it should remain the case that the native fee
token is cheaper, so that the majority of users will use the same fee token and
their transactions will not be distinguishable. On the other hand, letting it
be possible to use non native fee tokens means that users who send IBC tokens
into Penumbra can access the protocol, and even if one of their first acts is
to swap some de minimis amount of the native token, they can do that within the
system.
This implementation currently does not properly handle (B), in that it silently
burns all fees, regardless of token. It is not appropriate to burn any tokens
other than the native token, whose supply is managed by the chain. Instead, the
transaction-level fee check should move into
check_and_execute, and call amethod on the fee component that records the fee amount in the object store. At
the end of the block, all non-native fees should be swapped into the native
token and then burned. Emitted events should record:
EventFeefor each transaction, with the transaction's gas cost, the gasprices used to compute the base fee, the base fee, and the actual fee;
EventFeeBurnat the end of the block, with the total amount of nativefees burned as well as, for each alt fee token used to pay fees in that
block, how much of the alt token was paid and how much of the native token it
was traded for.
Client support is still mostly missing, though I have manually tested this on a
local devnet, by
pd testnet generate)dexParams.fixedAltGasPricesto"[{\"assetId\":{\"altBaseDenom\":\"ugm\"},\"blockSpacePrice\":\"80000\",\"compactBlockSpacePrice\":\"80000\",\"verificationPrice\":\"80000\",\"executionPrice\":\"80000\"}]"TxCmd::execa change to thegas_pricesto add thegmasset ID and multiplying all the prices by 10.To properly support clients, we need to
Record the
gas_pricesdata for alt fee tokens that is newly included in theCompactBlockin the view server's storage (probably in a newgas_pricestable that maps a fee token asset ID to the latest price);
Use that data to populate the view server's gas prices RPC response (instead
of the current
Vec::newstub that reports any alt fee tokens asnon-existing);
Change the
pcli txcommand so that the existing--fee-tierand a new--fee-tokenparameters move topcli tx, and apply to all subcommands(i.e.,
pcli tx --fee-tier ... --fee-token gm send ...) so thatTxCmd::execcan pull the right
GasPricesout of the RPC response.Checklist before requesting a review
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: