Separate bitcoin and misc fee levels #17156
Conversation
enjojoy
force-pushed
the
refactor-fee-levels
branch
from
a27ecb6 to
7c10ee9
Compare
enjojoy
requested review from
mroz22,
szymonlesisz,
martykan and
marekrjpolak
as code owners
enjojoy
force-pushed
the
refactor-fee-levels
branch
from
7c10ee9 to
3113266
Compare
WalkthroughThis pull request introduces several modifications across fee estimation and Bitcoin transaction modules. In the ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
@coderabbitai review |
|
@coderabbitai summary |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| return blocks.indexOf(lower); | ||
| }; | ||
|
|
||
| export class MiscFeeLevels { |
There was a problem hiding this comment.
This misc looks a lot ethereum specific.
Maybe there should be some generic implementation and other should inherit from it or override it?
MiscFeeLevels (the generic one or even one more), BitcoinFeeLevels, EthereumFeeLevels
There was a problem hiding this comment.
I thought about this too for the future though, but I agree that it already looks too Ethereum specific that it makes sense to separate it even now.
I'll do it then
There was a problem hiding this comment.
Yeah, we've discussed this and you're right, but this was ok for #16342 and also it somehow keep the previous naming (load vs loadMisc) 🤷
enjojoy
force-pushed
the
refactor-fee-levels
branch
from
3113266 to
83bc6d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/connect/src/api/ethereum/EthereumFees.ts (1)
9-17: Consider adding defensive checks infindBlocksForFee.While the logic is straightforward, you might want to handle potential edge cases (e.g., invalid string inputs that can’t be parsed as numbers). Ensuring graceful fallback or explicit error message can safeguard against unexpected data.
packages/connect/src/types/fees.ts (1)
11-17: Consider numeric types for gas fee fields.Using string types for gas fee values is consistent with other parts of the codebase, but numeric types might simplify validation. If switching is non-trivial, adding a note or constraint checks could help ensure correctness.
packages/connect/src/api/common/MiscFees.ts (2)
10-18: Consolidate duplicate logic infindBlocksForFee.This function matches the one in
EthereumFees.ts. Consider centralizing the logic in a shared utility to reduce code duplication and simplify maintenance.
33-55: Consider logging fee estimation errors.Currently, errors in
loadare silently caught, potentially making debugging difficult. Logging or callback-based error reporting could improve visibility into fee estimation failures.packages/connect/src/types/api/__tests__/blockchain.ts (1)
20-22: Ensure consistent error handling across fee-related properties.While
feePerUnituses optional chaining,feeLimitandfeePerTxuse inconsistent casing in their method calls (toLowerCasevstoLocaleLowerCase). Let's make the error handling and method usage consistent.- level.feePerUnit?.toLowerCase(); - level.feeLimit?.toLocaleLowerCase(); - level.feePerTx?.toLocaleLowerCase(); + level.feePerUnit?.toLocaleLowerCase(); + level.feeLimit?.toLocaleLowerCase(); + level.feePerTx?.toLocaleLowerCase();packages/connect/src/api/bitcoin/BitcoinFees.ts (2)
102-105: Improve null safety in fee rate conversion.The optional chaining on
feePerUnitis good, but passing '0' as a fallback might lead to unexpected behavior. Consider using a more explicit fallback value or error handling.- feePerUnit ?? '0', + feePerUnit ?? this.coinInfo.minFee.toString(),
124-134: Add JSDoc documentation for updateBitcoinCustomFee method.The method lacks documentation explaining its purpose, parameters, and behavior.
+ /** + * Updates the fee levels with a custom fee rate for Bitcoin transactions. + * @param feePerUnit - The custom fee rate in satoshis per byte + */ updateBitcoinCustomFee(feePerUnit: string) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/blockchain-link-types/src/responses.ts(2 hunks)packages/connect/src/api/bitcoin/BitcoinFees.ts(4 hunks)packages/connect/src/api/bitcoin/TransactionComposer.ts(5 hunks)packages/connect/src/api/bitcoin/__tests__/BitcoinFees.test.ts(4 hunks)packages/connect/src/api/bitcoin/index.ts(1 hunks)packages/connect/src/api/blockchainEstimateFee.ts(2 hunks)packages/connect/src/api/common/MiscFees.ts(1 hunks)packages/connect/src/api/ethereum/EthereumFees.ts(1 hunks)packages/connect/src/types/api/__tests__/blockchain.ts(1 hunks)packages/connect/src/types/fees.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/connect/src/api/bitcoin/tests/BitcoinFees.test.ts
- packages/connect/src/api/bitcoin/index.ts
- packages/connect/src/api/blockchainEstimateFee.ts
- packages/connect/src/api/bitcoin/TransactionComposer.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: build-web
🔇 Additional comments (7)
packages/connect/src/api/ethereum/EthereumFees.ts (2)
19-28: Verify thatcoinInfo.defaultFeesis always defined.Relying on
coinInfo.defaultFeescan lead to undefined behavior if this property is absent in some network configurations. Consider defaulting to an empty array or providing a fallback.
88-105: Validate custom fee ranges.The custom fee logic does not clamp or validate
feePerUnitoreffectiveGasPrice, which may lead to out-of-range values. Consider verifying or adjusting the inputs here, or at least warning the user if the value can cause transaction failures.packages/connect/src/types/fees.ts (1)
32-37: LGTM on added optional gas fee fields.These additions cover broader gas fee details and align well with EIP-1559 requirements, providing flexibility without breaking existing functionality.
packages/connect/src/api/common/MiscFees.ts (2)
1-2: Verify license and attribution for external code references.The comment references an external source. Verify that any necessary licensing or attribution is handled correctly to avoid potential legal or compliance issues.
57-67: Validate custom fee values before applying them.As noted previously, large or invalid fee values can result in unexpected transaction failures. Implementing range checks or user warnings can enhance reliability and user experience.
packages/blockchain-link-types/src/responses.ts (1)
2-2:❓ Verification inconclusive
Verify the impact of EstimateFee interface changes.
The change from a custom payload type to
WsEstimateFeeRes[]is a breaking change that could affect consumers of this interface.Also applies to: 98-98
🏁 Script executed:
#!/bin/bash # Search for direct usage of EstimateFee interface echo "Searching for EstimateFee interface usage..." rg -l "EstimateFee" --type ts # Search for direct access to feePerUnit, feePerTx, or feeLimit properties echo "Searching for direct property access..." rg "payload\.(feePerUnit|feePerTx|feeLimit)" --type tsLength of output: 2221
Breaking Change Alert: Verify EstimateFee Consumers
The change replacing the custom payload type with
WsEstimateFeeRes[]in theEstimateFeeinterface is acknowledged as a breaking change. Our shell script search shows that theEstimateFeeinterface is used in several parts of the codebase; however, we did not find direct accesses to properties likefeePerUnit,feePerTx, orfeeLimit. This suggests that consumers might not be relying on those specific properties, but the interface change still may have broader implications.Please manually verify that all consumers of
EstimateFeecorrectly handle the newWsEstimateFeeRes[]structure and that the updated type behavior is in line with expectations.packages/connect/src/api/bitcoin/BitcoinFees.ts (1)
59-60: LGTM! Good class renaming.The renaming from
FeeLevelstoBitcoinFeeLevelsand type update toBitcoinNetworkInfobetter reflects the class's specific responsibility, improving code clarity and maintainability.
| async load(blockchain: Blockchain) { | ||
| try { | ||
| const [response] = await blockchain.estimateFee({ blocks: [1] }); | ||
| if (response.eip1559) { | ||
| type EipResponse1559Level = 'low' | 'medium' | 'high'; | ||
| type Eip1559Level = 'low' | 'normal' | 'high'; | ||
| const eip1559ResponseLevelKeys = [ | ||
| 'low', | ||
| 'medium', | ||
| 'high', | ||
| ] as EipResponse1559Level[]; | ||
|
|
||
| const { eip1559 } = response; | ||
| const eip1559Levels = eip1559ResponseLevelKeys.map(levelKey => { | ||
| const level = eip1559[levelKey]; | ||
|
|
||
| // We can't pass BaseFeePerGas to firmware, so we calculate the effective gas price here | ||
| const calculatedMaxFeePerGas = BigNumber.minimum( | ||
| new BigNumber(level?.maxFeePerGas || '0'), | ||
| new BigNumber(eip1559.baseFeePerGas || '0').plus( | ||
| level?.maxPriorityFeePerGas || '0', | ||
| ), | ||
| ).toFixed(); | ||
|
|
||
| const label = | ||
| levelKey === 'medium' | ||
| ? ('normal' as Eip1559Level) | ||
| : (levelKey as Eip1559Level); | ||
|
|
||
| return { | ||
| label, | ||
| maxFeePerGas: level?.maxFeePerGas || '0', | ||
| effectiveGasPrice: calculatedMaxFeePerGas, | ||
| maxPriorityFeePerGas: level?.maxPriorityFeePerGas || '0', | ||
| baseFeePerGas: eip1559.baseFeePerGas, | ||
| minWaitTimeEstimate: level?.minWaitTimeEstimate | ||
| ? level.minWaitTimeEstimate / 1000 | ||
| : undefined, // Infura provides wait time in miliseconds | ||
| maxWaitTimeEstimate: level?.maxWaitTimeEstimate | ||
| ? level.maxWaitTimeEstimate / 1000 | ||
| : undefined, | ||
| feePerUnit: '0', | ||
| feeLimit: response.feeLimit, | ||
| blocks: -1, | ||
| }; | ||
| }); | ||
|
|
||
| this.levels = [...eip1559Levels]; | ||
| } else { | ||
| super.load(blockchain); | ||
| } | ||
| } catch { | ||
| // silent | ||
| } | ||
|
|
||
| return this.levels; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle and log errors in the load method instead of silently catching.
Swallowing errors without providing logs or rethrows can obscure issues if fee estimation fails (e.g., if response is undefined). Consider logging or re-throwing to help with debugging.
enjojoy
force-pushed
the
refactor-fee-levels
branch
3 times, most recently
from
34fae96 to
6c7361a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/connect/src/api/ethereum/EthereumFees.ts (2)
9-17: Fix typo in comment.The function implementation is solid, but there's a small typo in the comment.
- // if not found get latest know value + // if not found get latest known value
88-104: Add input validation for fee parameters.Consider validating the input parameters to ensure they are non-negative and properly formatted BigNumber strings.
updateEthereumCustomFee( feePerUnit: string, effectiveGasPrice?: string, maxPriorityFeePerGas?: string, ) { + // Validate input parameters + if (!BigNumber.isBigNumber(feePerUnit) || new BigNumber(feePerUnit).isNegative()) { + throw new Error('Invalid feePerUnit: must be a non-negative BigNumber string'); + } + if (effectiveGasPrice && (!BigNumber.isBigNumber(effectiveGasPrice) || new BigNumber(effectiveGasPrice).isNegative())) { + throw new Error('Invalid effectiveGasPrice: must be a non-negative BigNumber string'); + } + if (maxPriorityFeePerGas && (!BigNumber.isBigNumber(maxPriorityFeePerGas) || new BigNumber(maxPriorityFeePerGas).isNegative())) { + throw new Error('Invalid maxPriorityFeePerGas: must be a non-negative BigNumber string'); + } + // remove "custom" level from list this.levels = this.levels.filter(l => l.label !== 'custom');packages/connect/src/api/bitcoin/BitcoinFees.ts (2)
10-58: Consider adding TypeScript type annotations to utility functions.The utility functions are well-implemented but could benefit from explicit type annotations for better type safety and documentation.
-const convertFeeRate = (fee: string, minFee: number) => { +const convertFeeRate = (fee: string, minFee: number): string | undefined => { -const fillGap = (from: number, step: number, size: number) => { +const fillGap = (from: number, step: number, size: number): number[] => { -const findNearest = (requested: number, blocks: Blocks) => { +const findNearest = (requested: number, blocks: Blocks): string | undefined => { -const findLowest = (blocks: Blocks) => +const findLowest = (blocks: Blocks): string | undefined =>
72-122: Consider refactoring theloadmethod for better maintainability.The method is complex and handles multiple responsibilities. Consider breaking it down into smaller, focused functions:
- Block calculation
- Fee estimation
- Level updates
async load(blockchain: Blockchain) { + const calculateBlocks = () => { let blocks = fillGap(0, 1, 10); if (this.levels.length > 1) { blocks = this.levels .map(l => l.blocks) .reduce((result: number[], bl: number) => { if (result.length === 0) return result.concat([bl]); const from = result[result.length - 1]; const gap = bl - from; const incr = gap <= 30 ? 1 : 6; const fill = fillGap(from, incr, gap); return result.concat(fill); }, []); } const oneDayBlocks = 6 * 24; blocks.push(...fillGap(oneDayBlocks, oneDayBlocks / 2, oneDayBlocks * 6)); return blocks; + } + const updateFeeLevels = (response: Array<{ feePerUnit: string }>, blocks: number[]) => { response.forEach(({ feePerUnit }, index) => { this.blocks[blocks[index]] = convertFeeRate( feePerUnit || '0', this.coinInfo.minFee, ); }); this.levels.forEach(level => { const updatedValue = findNearest(level.blocks, this.blocks); if (typeof updatedValue === 'string') { level.blocks = this.blocks.indexOf(updatedValue); level.feePerUnit = updatedValue; } }); this.longTermFeeRate = findLowest(this.blocks); + } + const blocks = calculateBlocks(); try { const response = await blockchain.estimateFee({ blocks }); - response.forEach(({ feePerUnit }, index) => { - this.blocks[blocks[index]] = convertFeeRate( - feePerUnit || '0', - this.coinInfo.minFee, - ); - }); - - this.levels.forEach(level => { - const updatedValue = findNearest(level.blocks, this.blocks); - if (typeof updatedValue === 'string') { - level.blocks = this.blocks.indexOf(updatedValue); - level.feePerUnit = updatedValue; - } - }); - - this.longTermFeeRate = findLowest(this.blocks); + updateFeeLevels(response, blocks); } catch { - // do not throw + // Log error or provide context about the failure + console.warn('Failed to estimate Bitcoin fees'); } return this.levels; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/blockchain-link-types/src/responses.ts(2 hunks)packages/connect/src/api/bitcoin/BitcoinFees.ts(4 hunks)packages/connect/src/api/bitcoin/TransactionComposer.ts(5 hunks)packages/connect/src/api/bitcoin/__tests__/BitcoinFees.test.ts(4 hunks)packages/connect/src/api/bitcoin/index.ts(1 hunks)packages/connect/src/api/blockchainEstimateFee.ts(2 hunks)packages/connect/src/api/common/MiscFees.ts(1 hunks)packages/connect/src/api/ethereum/EthereumFees.ts(1 hunks)packages/connect/src/types/api/__tests__/blockchain.ts(1 hunks)packages/connect/src/types/fees.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/connect/src/api/bitcoin/index.ts
- packages/connect/src/types/api/tests/blockchain.ts
- packages/connect/src/api/bitcoin/tests/BitcoinFees.test.ts
- packages/blockchain-link-types/src/responses.ts
- packages/connect/src/api/bitcoin/TransactionComposer.ts
- packages/connect/src/api/common/MiscFees.ts
- packages/connect/src/api/blockchainEstimateFee.ts
- packages/connect/src/types/fees.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
🔇 Additional comments (6)
packages/connect/src/api/ethereum/EthereumFees.ts (3)
1-7: LGTM! Clean imports and type definitions.The imports are well-organized and the type definitions are clear and purposeful.
19-28: LGTM! Well-structured class definition.The class is well-designed with proper inheritance and initialization.
30-86: Handle and log errors in theloadmethod instead of silently catching.Swallowing errors without providing logs or rethrows can obscure issues if fee estimation fails (e.g., if
responseis undefined). Consider logging or re-throwing to help with debugging.packages/connect/src/api/bitcoin/BitcoinFees.ts (3)
6-7: LGTM! Good separation of concerns.The import changes align well with the PR objective of separating Bitcoin and misc fee levels. Moving
findBlocksForFeeto a dedicatedMiscFeesmodule improves code organization.
59-70: LGTM! Clear and focused class definition.The class renaming and type changes effectively narrow the scope to Bitcoin-specific fee management.
124-134: LGTM! Clear and focused method implementation.The method renaming and implementation align well with the Bitcoin-specific focus of the class.
enjojoy
force-pushed
the
refactor-fee-levels
branch
from
6c7361a to
36533a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/connect/src/api/bitcoin/BitcoinFees.ts (1)
124-134: 💡 Verification agent🧩 Analysis chain
Verify the impact of renaming updateCustomFee.
The method rename to
updateBitcoinCustomFeeimproves clarity but might affect existing code. Please verify all call sites have been updated.
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old method name rg "updateCustomFee" --type tsLength of output: 422
Action Required: Update Call Sites for Renamed Method
The renaming of the method in
packages/connect/src/api/bitcoin/BitcoinFees.tsfromupdateCustomFeetoupdateBitcoinCustomFeeimproves clarity but has impacted consistency across the codebase. Our grep search shows several remaining references to the old name:
- packages/connect-popup/src/index.tsx: Calls
view.updateCustomFee(message.payload)- packages/connect-popup/src/view/selectFee.ts & index.ts: Exports and uses
updateCustomFee- packages/connect/src/api/common/MiscFees.ts: Still defines
updateCustomFeePlease verify that these call sites are meant for the Bitcoin fees update. If so, update them to use
updateBitcoinCustomFeeor provide an appropriate backward-compatible alias to avoid runtime errors.
🧹 Nitpick comments (1)
packages/connect/src/api/blockchainEstimateFee.ts (1)
77-90: Consider extracting getFees to a utility file.The implementation correctly handles fee level instantiation based on coin type. However, to improve maintainability and follow the Single Responsibility Principle, consider moving this function to a separate utility file.
Create a new file
utils/feeUtils.ts:import { BitcoinFeeLevels } from '../bitcoin/BitcoinFees'; import { MiscFeeLevels } from '../common/MiscFees'; import { EthereumFeeLevels } from '../ethereum/EthereumFees'; import type { CoinInfo } from '../../types'; export const getFees = (coinInfo: CoinInfo) => { switch (coinInfo.type) { case 'bitcoin': return new BitcoinFeeLevels(coinInfo); case 'ethereum': return new EthereumFeeLevels(coinInfo); default: return new MiscFeeLevels(coinInfo); } };🧰 Tools
🪛 Biome (1.9.4)
[error] 88-88: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/blockchain-link-types/src/responses.ts(2 hunks)packages/connect/src/api/bitcoin/BitcoinFees.ts(4 hunks)packages/connect/src/api/bitcoin/TransactionComposer.ts(5 hunks)packages/connect/src/api/bitcoin/__tests__/BitcoinFees.test.ts(4 hunks)packages/connect/src/api/bitcoin/index.ts(1 hunks)packages/connect/src/api/blockchainEstimateFee.ts(2 hunks)packages/connect/src/api/common/MiscFees.ts(1 hunks)packages/connect/src/api/ethereum/EthereumFees.ts(1 hunks)packages/connect/src/types/api/__tests__/blockchain.ts(1 hunks)packages/connect/src/types/fees.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/connect/src/types/api/tests/blockchain.ts
- packages/connect/src/api/bitcoin/index.ts
- packages/blockchain-link-types/src/responses.ts
- packages/connect/src/api/bitcoin/tests/BitcoinFees.test.ts
- packages/connect/src/api/common/MiscFees.ts
- packages/connect/src/types/fees.ts
- packages/connect/src/api/ethereum/EthereumFees.ts
- packages/connect/src/api/bitcoin/TransactionComposer.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/connect/src/api/blockchainEstimateFee.ts
[error] 88-88: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build-deploy
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
- GitHub Check: prepare_android_test_app
🔇 Additional comments (3)
packages/connect/src/api/blockchainEstimateFee.ts (1)
9-11: LGTM! Clean separation of fee level classes.The imports reflect a clear separation of concerns between different blockchain fee levels, aligning with the PR objective.
packages/connect/src/api/bitcoin/BitcoinFees.ts (2)
6-7: LGTM! Improved type safety with Bitcoin-specific types.The changes enhance type safety by using Bitcoin-specific network information types and properly importing shared utility functions.
Also applies to: 59-60
67-70: LGTM! Constructor properly typed for Bitcoin network info.The constructor correctly uses the Bitcoin-specific network information type, maintaining type safety.
Description