-
Notifications
You must be signed in to change notification settings - Fork 15
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
Support fees in Penumbra web #362
Conversation
...props | ||
}: InputTokenProps) { | ||
const vResult = validationResult(value, validations); | ||
|
||
const currentBalance = getCurrentBalance(selection?.asset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted to a helper to make it clear what this represents.
apps/webapp/public/fuel.svg
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got this from Lucide, which Zpoken told us is where we got some of our icons.
It'd be great to eventually standardize an icon set, though — right now, we seem to be getting icons from Radix, Lucide, and our Figma mockups (and I don't know where those mockups' icons come from).
@@ -82,7 +89,7 @@ export const SendForm = () => { | |||
}, | |||
]} | |||
balances={accountBalances} | |||
tempPrice={1} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my note below about removing the temp price and its corresponding UI
import type { Impl } from '.'; | ||
|
||
export const gasPrices: Impl['gasPrices'] = () => { | ||
/** @todo Replace this stub with real gas prices. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Valentine1898 is currently working on this as part of #203
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that gas prices do not actually change on the network and are always equal to these values
blockSpacePrice: 0n
compactBlockSpacePrice: 0n
executionPrice: 0n
verificationPrice: 0n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Valentine1898 but that could change in the future, right? i.e., I should still be requesting them (using your new implementation) rather rather than depending on all-0 values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was just clarifying that we're currently getting all the 0 values from the network
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, thanks
apps/webapp/src/state/send.ts
Outdated
return ( | ||
gasPrices.blockSpacePrice + | ||
gasPrices.compactBlockSpacePrice + | ||
gasPrices.verificationPrice + | ||
gasPrices.executionPrice | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm summing up the different dimensions of the gas price to get the total. I'm not 100% certain that this logic is correct — this is where I'm bumping against the limits of my crypto knowledge — so reviewers, please let me know if I've got this right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like the final step of calculating a total might be the location where the denominator is expected to be considered
https://github.com/penumbra-zone/penumbra/blob/main/crates/core/component/fee/src/gas.rs#L65-L74
)} | ||
> | ||
${displayAmount(Number(value) * tempPrice)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, this appeared to be a stub of the gas price. (I'm not sure of that, though — I'm not actually sure what tempPrice
was for.) So I removed it and displayed the gas price in its place. If I'm missing something about the tempPrice
, let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the way i would handle this, is instead of having a state selector that provides the total, provide a state selector that provides all of the price components. that way it could be displayed differently, or with more detail.
otherwise looks like good progress, pending the wasm impl
apps/webapp/src/state/send.ts
Outdated
return ( | ||
gasPrices.blockSpacePrice + | ||
gasPrices.compactBlockSpacePrice + | ||
gasPrices.verificationPrice + | ||
gasPrices.executionPrice | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like the final step of calculating a total might be the location where the denominator is expected to be considered
https://github.com/penumbra-zone/penumbra/blob/main/crates/core/component/fee/src/gas.rs#L65-L74
403fb83
to
6163c32
Compare
how to calculate gas price preview on send form gas prices can be calculated from transaction plan information. the planner performs this calculation here https://github.com/penumbra-zone/penumbra/blob/main/crates/view/src/planner.rs#L157-L174 the transactionplan's impl simply sums the costs of the action plans https://github.com/penumbra-zone/penumbra/blob/main/crates/core/transaction/src/gas.rs#L225-L229 the send form only ever generates 'spend' and 'output' actions. cost for these plans are constant, based on their constant dimensions https://github.com/penumbra-zone/penumbra/blob/main/crates/core/transaction/src/gas.rs#L23-L65 the final cost can be estimated using price information from GasPriceRequest https://github.com/penumbra-zone/penumbra/blob/main/crates/core/component/fee/src/gas.rs#L65-L74 |
It'd be strongly preferable not to replicate gas cost estimation logic in typescript. We will already be doing exact gas costing as part of the planner. Can we instead just read the fee out of the returned transaction plan? It is actually reasonably complicated to estimate the gas cost for a particular action, because the frontend isn't in a position to know e.g., how many notes need to be spent to create a plan that fufils the user's intent. (What if a user's balance is spread over many small notes)? Implementing all of this is fairly close to implementing the transaction planner logic itself. Instead, what if the form submitted planner requests to the extension as the values were input, so it can display a preview, but have the logic for computing the preview be the actual logic that will be used for tx planning? |
the ticket requested displaying a preview in the webapp, by our interpretation. displaying in the popup is certainly much easier :) |
or, should we have an extra stage between "plan came back" and "submit plan for auth" |
@turbocrime added an extra suggestion to my comment in an edit, i think a preview is certainly nice to have, wdyt of that strategy for having it? the point is that the view service methods don't have user in the loop, the page can make as many planner requests as it wants and throw away the resulting plans |
current design would require sending a new plan request on every change, creating a new planner and re-planning the whole tx each time, instead of incrementally planning a single tx |
very doable |
my assumption is that planning should be pretty fast, fast enough that we could just spam requests on form change, it seems at least worth trying |
the existing webapp combines plan/witness/build/broadcast into a single fn and i think we were conceptually trapped lol |
Maybe it makes sense to leave it that way and send separate planner requests for gas cost preview |
Thanks all — making planner requests for the gas costs sounds great. I'll go with that approach! |
bbd97d7
to
3975294
Compare
import { useStore } from '../../../state'; | ||
|
||
/** | ||
* Refreshes the fee in the state when the amount, recipient, selection, or memo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we're refreshing even when just the memo changes. Am I correct in understanding that a larger memo could result in the fee changing, since a larger memo takes up more space? Or does the fee remain the same regardless of memo?
If the latter, I'll remove memo
from the list of dependencies on line 14 below, since we don't need to refresh the fee when the memo changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe an encrypted memo is constant size, even if it contains no memo
additionally, when the planner calculates tx cost it does not use the memo, it uses the list of actions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks — I've removed memo
from the effect dependencies.
import { Selection } from '../../state/types'; | ||
import { Fee } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/component/fee/v1alpha1/fee_pb'; | ||
|
||
const PENUMBRA_FEE_DENOMINATOR = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we're currently hardcoding the penumbra fee denominator. @Valentine1898 pointed out that I could call getAllAssets
to get the denominator of the token referenced via the fee's assetId
property; but @hdevalence, will there ever be a case where the fee will be paid in something other than the staking token (penumbra)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe currently tx fees are always PEN https://protocol.penumbra.zone/main/stake/validator-rewards.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://discord.com/channels/824484045370818580/979785365437165658/1194805904311328808
We can ignore the assetId
for now
apps/webapp/src/state/send.ts
Outdated
if (!selection.asset) throw new Error('no selected asset'); | ||
type TransactionProps = Pick<SendSlice, 'amount' | 'recipient' | 'selection' | 'memo'>; | ||
|
||
const getPlan = async ({ amount, recipient, selection, memo }: TransactionProps) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted a getPlan
function, which we call to refresh the fee whenever a form input changes.
apps/webapp/src/state/send.ts
Outdated
return plan; | ||
}; | ||
|
||
const planWitnessBuildBroadcast = async ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works exactly as before, even throwing the same errors as before. I've only extracted a getPlan
function above.
8191b14
to
96585fa
Compare
import { Selection } from '../../state/types'; | ||
import { Fee } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/component/fee/v1alpha1/fee_pb'; | ||
|
||
const PENUMBRA_FEE_DENOMINATOR = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://discord.com/channels/824484045370818580/979785365437165658/1194805904311328808
We can ignore the assetId
for now
31e2ace
to
c253544
Compare
1aebc54
to
f389249
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments, feel free to merge after 👍
export const useRefreshFee = () => { | ||
const { amount, recipient, selection, refreshFee } = useStore(sendSelector); | ||
const timeoutId = useRef<number | null>(null); | ||
|
||
const debouncedRefreshFee = useCallback(() => { | ||
if (timeoutId.current) { | ||
window.clearTimeout(timeoutId.current); | ||
timeoutId.current = null; | ||
} | ||
|
||
timeoutId.current = window.setTimeout(() => { | ||
timeoutId.current = null; | ||
void refreshFee(); | ||
}, DEBOUNCE_MS); | ||
}, [refreshFee]); | ||
|
||
useEffect(debouncedRefreshFee, [amount, recipient, selection, debouncedRefreshFee]); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it may be better as a selector. Maybe not though 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Selectors always have to be synchronous, pure functions, no? Or how are you suggesting accomplishing this with a selector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed in Discord — this will be part of a follow-up PR
const getCurrentBalance = (assetBalance: AssetBalance | undefined) => | ||
assetBalance | ||
? fromBaseUnitAmount(assetBalance.amount, assetBalance.denom.exponent).toFormat() | ||
: '0'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use ValueViewComponent
here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed in Discord — this will be part of a follow-up PR (in progress here #440)
const fee = joinLoHiAmount(txv.bodyView.transactionParameters!.fee!.amount!).toString(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think we should have error guards here:
if (!txv.bodyView?.transactionParameters?.fee?.amount) throw new Error('Missing fee amount');
const fee = joinLoHiAmount(txv.bodyView.transactionParameters.fee.amount).toString();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call — fixed
f0efaad
to
72a046c
Compare
Background
Per #308, we want to support transaction fees in Penumbra Web. To do this, we need to make a
TransactionPlannerRequest
each time one of the values in the send form changes, then grab the gas fee off of the returned plan.In this PR
planWitnessBuildBroadcast
in theSendSlice
to extract agetPlan
function that takes care of just the planning portion.getPlan
function to get the gas fee any time inputs change in the send form.Closes #308