-
Notifications
You must be signed in to change notification settings - Fork 15
wip for a possible fix where celo receives undervalued gas estimates #522
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1203,6 +1203,18 @@ export class Web3Wallet { | |
| } | ||
| } | ||
|
|
||
| pickFeeWithFloor(value: string | number | void, floor: string | number | void) { | ||
| if (value === null || value === undefined) { | ||
| return floor | ||
| } | ||
|
|
||
| if (floor === null || floor === undefined) { | ||
| return value | ||
| } | ||
|
|
||
| return BigInt(value.toString()) < BigInt(floor.toString()) ? floor : value | ||
| } | ||
|
|
||
| /** | ||
| * Normalize gas pricing parameters - handles legacy gasPrice vs EIP-1559 fees | ||
| * Deduplicates gas pricing logic used in sendTransaction and sendNative | ||
|
|
@@ -1233,24 +1245,36 @@ export class Web3Wallet { | |
| }) | ||
| return { gasPrice, maxFeePerGas: undefined, maxPriorityFeePerGas: undefined } | ||
| } else { | ||
| // Network supports EIP-1559, process EIP-1559 fees | ||
| // Use instance defaults if not provided | ||
| maxFeePerGas = maxFeePerGas !== undefined ? maxFeePerGas : this.maxFeePerGas | ||
| maxPriorityFeePerGas = maxPriorityFeePerGas !== undefined ? maxPriorityFeePerGas : this.maxPriorityFeePerGas | ||
| // Network supports EIP-1559, process EIP-1559 fees. | ||
| // Instance values are treated as floors so live provider estimates can rise above them. | ||
| const maxFeeFloor = maxFeePerGas !== undefined ? maxFeePerGas : this.maxFeePerGas | ||
| const maxPriorityFloor = maxPriorityFeePerGas !== undefined ? maxPriorityFeePerGas : this.maxPriorityFeePerGas | ||
|
|
||
| const needsEstimation = !maxFeePerGas || !maxPriorityFeePerGas | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (bug_risk): The Previously, |
||
|
|
||
| if (needsEstimation) { | ||
| try { | ||
| const { baseFee, priorityFee } = await this.getFeeEstimates() | ||
| maxFeePerGas = this.pickFeeWithFloor(baseFee, maxFeeFloor) | ||
| maxPriorityFeePerGas = this.pickFeeWithFloor(priorityFee, maxPriorityFloor) | ||
| } catch (error) { | ||
| logger.warn('Failed to estimate EIP-1559 fees, falling back to configured values', { | ||
| error: error.message, | ||
| network: this.network, | ||
| networkId: this.networkId | ||
| }) | ||
| maxFeePerGas = maxFeeFloor | ||
| maxPriorityFeePerGas = maxPriorityFloor | ||
| } | ||
| } else { | ||
| maxFeePerGas = maxFeeFloor | ||
| maxPriorityFeePerGas = maxPriorityFloor | ||
| } | ||
|
|
||
| // Convert to numbers for comparison | ||
| let maxFeeNum = toNumber(maxFeePerGas) | ||
| let maxPriorityNum = toNumber(maxPriorityFeePerGas) | ||
|
Comment on lines
1275
to
1276
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (bug_risk): Loss of the previous With the removal of this guard, non-numeric |
||
|
|
||
| // Fetch fee estimates if EIP-1559 fees are not provided or invalid | ||
| if (!maxFeeNum || !maxPriorityNum) { | ||
| const { baseFee, priorityFee } = await this.getFeeEstimates() | ||
| maxFeePerGas = maxFeeNum || baseFee | ||
| maxPriorityFeePerGas = maxPriorityNum || priorityFee | ||
| maxFeeNum = toNumber(maxFeePerGas) | ||
| maxPriorityNum = toNumber(maxPriorityFeePerGas) | ||
| } | ||
|
|
||
| // Ensure maxFeePerGas >= maxPriorityFeePerGas (EIP-1559 requirement) | ||
| if (maxPriorityNum > 0 && (maxFeeNum === 0 || maxFeeNum < maxPriorityNum)) { | ||
| logger.warn('maxFeePerGas < maxPriorityFeePerGas or is 0, adjusting maxFeePerGas', { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| jest.mock('../../db/mongo-db') | ||
|
|
||
| import { Web3Wallet } from '../Web3Wallet' | ||
|
|
||
| const createTestWallet = options => | ||
| new Web3Wallet( | ||
| 'TestWallet', | ||
| { | ||
| env: 'test', | ||
| mnemonic: '', | ||
| network: 'test', | ||
| kmsEnabled: false, | ||
| fuse: { | ||
| network_id: 42220, | ||
| httpWeb3Provider: 'http://localhost:8545' | ||
| } | ||
| }, | ||
| { | ||
| ethereum: { | ||
| network_id: 42220, | ||
| httpWeb3Provider: 'http://localhost:8545' | ||
| }, | ||
| network: 'test-celo', | ||
| ...options | ||
| }, | ||
| false | ||
| ) | ||
|
|
||
| describe('Web3Wallet gas pricing', () => { | ||
| const logger = { | ||
| debug: jest.fn(), | ||
| warn: jest.fn() | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks() | ||
| }) | ||
|
|
||
| test('prefers provider EIP-1559 fees when they are above configured floors', async () => { | ||
| const wallet = createTestWallet({ | ||
| maxFeePerGas: '40000000000', | ||
| maxPriorityFeePerGas: '1000000000' | ||
| }) | ||
|
|
||
| wallet.supportsEIP1559 = jest.fn().mockResolvedValue(true) | ||
| wallet.getFeeEstimates = jest.fn().mockResolvedValue({ | ||
| baseFee: 52000000000, | ||
| priorityFee: 2000000000 | ||
|
Comment on lines
+39
to
+48
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (testing): Add tests for the branch where both EIP-1559 fees are fully provided and no estimation is performed. The current tests only cover cases where
This will cover the non-estimation path when callers fully specify fees. Suggested implementation: beforeEach(() => {
jest.clearAllMocks()
})
test('uses fully specified EIP-1559 fees without estimation', async () => {
const wallet = createTestWallet({
maxFeePerGas: '60000000000',
maxPriorityFeePerGas: '3000000000'
})
wallet.supportsEIP1559 = jest.fn().mockResolvedValue(true)
wallet.getFeeEstimates = jest.fn()
const txParams = {
maxFeePerGas: '60000000000',
maxPriorityFeePerGas: '3000000000'
}
const result = await wallet.getGasPricing(txParams, logger)
expect(wallet.getFeeEstimates).not.toHaveBeenCalled()
expect(result.maxFeePerGas).toBe('60000000000')
expect(result.maxPriorityFeePerGas).toBe('3000000000')
expect(BigInt(result.maxFeePerGas)).toBeGreaterThanOrEqual(BigInt(result.maxPriorityFeePerGas))
})
test('prefers provider EIP-1559 fees when they are above configured floors', async () => {
|
||
| }) | ||
|
|
||
| const gas = await wallet.normalizeGasPricing({}, logger) | ||
|
|
||
| expect(gas).toEqual({ | ||
| gasPrice: undefined, | ||
| maxFeePerGas: 52000000000, | ||
| maxPriorityFeePerGas: 2000000000 | ||
| }) | ||
| }) | ||
|
|
||
| test('uses configured floors when provider underbids Celo fees', async () => { | ||
| const wallet = createTestWallet({ | ||
| maxFeePerGas: '40000000000', | ||
| maxPriorityFeePerGas: '1000000000' | ||
| }) | ||
|
|
||
| wallet.supportsEIP1559 = jest.fn().mockResolvedValue(true) | ||
| wallet.getFeeEstimates = jest.fn().mockResolvedValue({ | ||
| baseFee: 30000000000, | ||
| priorityFee: 200000000 | ||
| }) | ||
|
|
||
| const gas = await wallet.normalizeGasPricing({}, logger) | ||
|
|
||
| expect(gas).toEqual({ | ||
| gasPrice: undefined, | ||
| maxFeePerGas: '40000000000', | ||
| maxPriorityFeePerGas: '1000000000' | ||
| }) | ||
| }) | ||
|
|
||
| test('falls back to configured floors when fee estimation fails', async () => { | ||
| const wallet = createTestWallet({ | ||
| maxFeePerGas: '40000000000', | ||
| maxPriorityFeePerGas: '1000000000' | ||
| }) | ||
|
|
||
| wallet.supportsEIP1559 = jest.fn().mockResolvedValue(true) | ||
| wallet.getFeeEstimates = jest.fn().mockRejectedValue(new Error('rpc failure')) | ||
|
|
||
| const gas = await wallet.normalizeGasPricing({}, logger) | ||
|
|
||
| expect(gas).toEqual({ | ||
| gasPrice: undefined, | ||
| maxFeePerGas: '40000000000', | ||
| maxPriorityFeePerGas: '1000000000' | ||
| }) | ||
| expect(logger.warn).toHaveBeenCalledWith('Failed to estimate EIP-1559 fees, falling back to configured values', { | ||
| error: 'rpc failure', | ||
| network: 'test-celo', | ||
| networkId: 42220 | ||
| }) | ||
| }) | ||
| }) | ||
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.
issue (complexity): Consider simplifying the EIP-1559 fee handling by removing
pickFeeWithFloorand doing all floor and estimate logic in a single numeric flow usingMath.maxandtoNumber.You can keep the “instance values as floors” behavior with a single, numeric flow and remove
pickFeeWithFloor. That reduces branching and type juggling without changing semantics.1. Drop
pickFeeWithFloorand use numeric comparisonInstead of mixing
BigInt,toString, and null-handling in a helper, normalize to numbers once and useMath.max:Inline floor application after estimates:
2. Keep normalization and validation in one place
Now the rest of the logic stays as-is and operates in one numeric domain:
This keeps the new behavior:
But it removes the extra helper, avoids BigInt juggling, and makes the flow linear: compute floors → optionally fetch estimates → apply floors vs estimates numerically → enforce EIP‑1559 constraint.