-
-
Notifications
You must be signed in to change notification settings - Fork 275
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
chore(suite): add priority fee network feature #16342
base: develop
Are you sure you want to change the base?
Conversation
🚀 Expo preview is ready!
|
1976ef5
to
51b93d9
Compare
export interface EstimateFee { | ||
type: typeof RESPONSES.ESTIMATE_FEE; | ||
payload: { | ||
feePerUnit: string; | ||
feePerTx?: string; | ||
feeLimit?: string; | ||
eip1559?: { |
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 be imported from Blockbook
const result = await TrezorConnect.blockchainEstimateFee({ | ||
coin: network.symbol, | ||
request: { | ||
blocks: [2], | ||
feeLevels: 'smart', |
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.
what is it?
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's needed for BlockchainEstimateFee method in connect. If the feeLevels are "smart", they are being created using FeeLevels.load method
which triggers loadMisc method that builds levels for 1559
It was there before but it wasn't implemented for ethereum
If they're not smart then it returns just one level which is the same as the response from blockbook
![Screenshot 2025-01-17 at 15 59 02](https://private-user-images.githubusercontent.com/101045932/404344986-fbecfbf3-f515-4882-94ed-b3304b33e387.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkzMzI2MDgsIm5iZiI6MTczOTMzMjMwOCwicGF0aCI6Ii8xMDEwNDU5MzIvNDA0MzQ0OTg2LWZiZWNmYmYzLWY1MTUtNDg4Mi05NGVkLWIzMzA0YjMzZTM4Ny5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjEyJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxMlQwMzUxNDhaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT0wMTg5MTc1YzQxNTU3MGRlZjFiOTUwNGQ4OThlODNlMzY2NjM5NWU1NzdlMTg2ODhmNGVjM2FkMTkzOWZmZDk4JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.Oq18HF7Z_8t8KApwWkUE6b7K-I-XDPhmzJeZCk_FZH4)
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.
aha, this issue is still open #9185 so I don't know if it will start working out of the box
if (response.eip1559) { | ||
const eip1559LevelKeys = ['low', 'medium', 'high'] as const; | ||
|
||
const { eip1559, ...baseLevel } = response; |
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.
what is this good for? ...baseLevel
{ ...baseLevel, label: 'normal' as const, blocks: -1, ethFeeType: 'legacy' }, | ||
...eip1559Levels, |
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 don't think this makes sense.
normal
level is probably low
. Lowest fee to make it through
How about, if there is eip1559 available, let's do something like this:
- low level eip1559 + normal legacy level
- medium level eip1559 + normal legacy level increased by some constant
- high level eip1559 + normal legacy level increased by some bigger constant
I am struggling if the constant is needed but probably yes as users can use old fw without eip1559 support.
If eip1559 is available for specific network, suite will check if eip1559 values are available and if not it will fallback to legacy.
But please wait for @martykan @marekrjpolak for their opinions
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 would just spread eip levels and add legacy gas there
8b615ab
to
a02ee59
Compare
FEE_LEVEL_MEDIUM: { | ||
defaultMessage: 'Medium', | ||
id: 'FEE_LEVEL_MEDIUM', | ||
}, |
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.
No need to introduce new level
@@ -6131,6 +6135,14 @@ export default defineMessages({ | |||
id: 'TR_DATA', | |||
defaultMessage: 'Data', | |||
}, | |||
TR_MAX_PRIORITY_FEE_PER_GAS: { | |||
id: 'TR_MAX_PRIORITY_FEE_PER_GAS', | |||
defaultMessage: 'Max priority fee per gas', |
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.
defaultMessage: 'Max priority fee per gas', | |
defaultMessage: 'Max priority fee', |
}, | ||
TR_MAX_FEE_PER_GAS: { | ||
id: 'TR_MAX_FEE_PER_GAS', | ||
defaultMessage: 'Max fee per gas', |
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.
defaultMessage: 'Max fee per gas', | |
defaultMessage: 'Max fee', |
@@ -34,12 +34,15 @@ export const allowedCardFrameProps = [ | |||
] as const satisfies FramePropsKeys[]; | |||
type AllowedFrameProps = Pick<FrameProps, (typeof allowedCardFrameProps)[number]>; | |||
|
|||
const Container = styled.div<{ $fillType: FillType } & TransientProps<AllowedFrameProps>>` | |||
const Container = styled.div< |
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.
probably some other (new) component than Card
should be used for this
...props | ||
}: FeesProps<TFieldValues>) => { | ||
// Type assertion allowing to make the component reusable, see https://stackoverflow.com/a/73624072. | ||
const shouldUsePriorityFees = network?.features?.includes('eip1559'); |
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.
const shouldUsePriorityFees = network?.features?.includes('eip1559'); | |
const shouldUsePriorityFees = getNetworkFeatures(symbol).includes("eip1559"); |
composedLevels?: PrecomposedLevels | PrecomposedLevelsCardano; | ||
label?: TranslationKey; | ||
rbfForm?: boolean; | ||
helperText?: React.ReactNode; | ||
network: 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.
network not needed here, you have account symbol
@@ -146,7 +146,7 @@ export const prepareEthereumTransaction = (txInfo: EthTransactionData) => { | |||
chainId: txInfo.chainId, | |||
nonce: numberToHex(txInfo.nonce), | |||
gasLimit: numberToHex(txInfo.gasLimit), | |||
gasPrice: numberToHex(toWei(txInfo.gasPrice, 'gwei')), | |||
gasPrice: numberToHex(toWei(txInfo.gasPrice || '0', 'gwei')), |
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.
In case of legacy, it should probably fail with gasPrice
undefined? It will fail with 0
anyways.
const feeInGwei = calculateEthFee(toWei(feeLevel.feePerUnit, 'gwei'), feeLevel.feeLimit || '0'); | ||
const feeInGwei = calculateEthFee( | ||
toWei(feeLevel.feePerUnit || '0', 'gwei'), | ||
feeLevel.feeLimit || '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.
note: feeLimit should probably not be undefined. never
{networkType === 'bitcoin' && <BitcoinDetails {...props} />} | ||
{networkType === 'ethereum' && <EthereumDetails {...props} />} | ||
{networkType === 'ripple' && <RippleDetails {...props} />} | ||
{networkType !== 'bitcoin' && networkType !== 'ethereum' && ( | ||
<MiscDetails {...props} /> | ||
)} |
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.
would be good to create switch
const theme = useTheme(); | ||
|
||
return ( | ||
<WrapperWithCheckmark selected={selected}> |
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 can be feature of the new component added to @trezor/component
a02ee59
to
bef5516
Compare
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
9062eec
to
816379a
Compare
816379a
to
f169878
Compare
f169878
to
5ee6a6e
Compare
5ee6a6e
to
25460db
Compare
Description
Related Issue
Resolve #16438
Screenshots: