wip for a possible fix where celo receives undervalued gas estimates#522
wip for a possible fix where celo receives undervalued gas estimates#522L03TJ3 wants to merge 1 commit into
Conversation
|
@copilot can you review the proposed fix and perhaps conclude why or how we could end up with different estimated gas values then when testing locally (looking at our use of getFeeEstimates in Web3Wallet.js) |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/server/blockchain/Web3Wallet.js" line_range="1253" />
<code_context>
+ const maxFeeFloor = maxFeePerGas !== undefined ? maxFeePerGas : this.maxFeePerGas
+ const maxPriorityFloor = maxPriorityFeePerGas !== undefined ? maxPriorityFeePerGas : this.maxPriorityFeePerGas
+
+ const needsEstimation = !maxFeePerGas || !maxPriorityFeePerGas
+
+ if (needsEstimation) {
</code_context>
<issue_to_address>
**issue (bug_risk):** The `needsEstimation` check now treats `'0'`/`0` differently than before and may skip estimation when values are numerically zero.
Previously, `toNumber` ensured that `0` and invalid values were treated as needing estimation via `if (!maxFeeNum || !maxPriorityNum)`. The new check `!maxFeePerGas || !maxPriorityFeePerGas` works on the raw inputs, so a string like `'0'` is truthy and will no longer trigger estimation. This changes behavior for callers passing zero-like values and may result in using a zero max fee/tip instead of estimating. If you want to preserve the old behavior (zero → re-estimate), `needsEstimation` should be based on numeric-converted values or an explicit zero check rather than the raw inputs.
</issue_to_address>
### Comment 2
<location path="src/server/blockchain/Web3Wallet.js" line_range="1275-1276" />
<code_context>
+ }
// Convert to numbers for comparison
let maxFeeNum = toNumber(maxFeePerGas)
let maxPriorityNum = toNumber(maxPriorityFeePerGas)
- // Fetch fee estimates if EIP-1559 fees are not provided or invalid
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Loss of the previous `!maxFeeNum || !maxPriorityNum` validation means NaN values are no longer corrected via estimation.
With the removal of this guard, non-numeric `maxFeePerGas` / `maxPriorityFeePerGas` can now become `NaN` via `toNumber` while `needsEstimation` is `false`, so they’ll flow into the comparison logic and downstream fee handling as `NaN`. To keep the prior safety behavior, add a post-conversion check (e.g. `!Number.isFinite(maxFeeNum)` or `maxFeeNum <= 0`, same for priority fee) and fall back to estimates or a safe default when the values are invalid.
</issue_to_address>
### Comment 3
<location path="src/server/blockchain/__tests__/web3wallet.gas.test.js" line_range="39-48" />
<code_context>
+ test('prefers provider EIP-1559 fees when they are above configured floors', async () => {
</code_context>
<issue_to_address>
**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 `needsEstimation` is `true` (at least one of `maxFeePerGas` / `maxPriorityFeePerGas` is missing). Please add a test that passes both values in `txParams` so `needsEstimation` is `false`, and verify that:
- `getFeeEstimates` is not called
- The returned `maxFeePerGas` / `maxPriorityFeePerGas` match the provided values (used as floors) and still satisfy the EIP-1559 constraint after adjustment.
This will cover the non-estimation path when callers fully specify fees.
Suggested implementation:
```javascript
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 () => {
```
- If the helper used in this file is not `wallet.getGasPricing(txParams, logger)` but some other function (e.g. a standalone `getGasPricing(wallet, txParams, logger)`), adjust the call accordingly while keeping the assertions identical.
- If `maxFeePerGas` / `maxPriorityFeePerGas` in the rest of the suite are `BigNumber`/`BN`/numeric types rather than strings, convert the literals and comparisons to match that convention, and adapt the EIP-1559 constraint assertion to use the same big-number utilities.
</issue_to_address>
### Comment 4
<location path="src/server/blockchain/Web3Wallet.js" line_range="1206" />
<code_context>
}
}
+ pickFeeWithFloor(value: string | number | void, floor: string | number | void) {
+ if (value === null || value === undefined) {
+ return floor
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the EIP-1559 fee handling by removing `pickFeeWithFloor` and doing all floor and estimate logic in a single numeric flow using `Math.max` and `toNumber`.
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 `pickFeeWithFloor` and use numeric comparison
Instead of mixing `BigInt`, `toString`, and null-handling in a helper, normalize to numbers once and use `Math.max`:
```js
// remove this entirely
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
}
```
Inline floor application after estimates:
```js
// 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 hasAnyFeeInput =
maxFeePerGas != null ||
maxPriorityFeePerGas != null
if (!hasAnyFeeInput) {
try {
const { baseFee, priorityFee } = await this.getFeeEstimates()
const baseFeeNum = toNumber(baseFee)
const priorityFeeNum = toNumber(priorityFee)
const maxFeeFloorNum = toNumber(maxFeeFloor)
const maxPriorityFloorNum = toNumber(maxPriorityFloor)
// estimates can rise above floors
const effectiveMaxFeeNum = Math.max(baseFeeNum, maxFeeFloorNum)
const effectiveMaxPriorityNum = Math.max(priorityFeeNum, maxPriorityFloorNum)
maxFeePerGas = effectiveMaxFeeNum.toString()
maxPriorityFeePerGas = effectiveMaxPriorityNum.toString()
} 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
}
```
### 2. Keep normalization and validation in one place
Now the rest of the logic stays as-is and operates in one numeric domain:
```js
let maxFeeNum = toNumber(maxFeePerGas)
let maxPriorityNum = toNumber(maxPriorityFeePerGas)
if (maxPriorityNum > 0 && (maxFeeNum === 0 || maxFeeNum < maxPriorityNum)) {
logger.warn('maxFeePerGas < maxPriorityFeePerGas or is 0, adjusting maxFeePerGas', {
originalMaxFeePerGas: maxFeePerGas,
maxPriorityFeePerGas: maxPriorityFeePerGas,
adjustedMaxFeePerGas: maxPriorityFeePerGas,
})
maxFeePerGas = maxPriorityFeePerGas
}
```
This keeps the new behavior:
- Instance values are floors.
- Estimates can exceed floors.
- On estimation failure, you fall back to floors.
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.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const maxFeeFloor = maxFeePerGas !== undefined ? maxFeePerGas : this.maxFeePerGas | ||
| const maxPriorityFloor = maxPriorityFeePerGas !== undefined ? maxPriorityFeePerGas : this.maxPriorityFeePerGas | ||
|
|
||
| const needsEstimation = !maxFeePerGas || !maxPriorityFeePerGas |
There was a problem hiding this comment.
issue (bug_risk): The needsEstimation check now treats '0'/0 differently than before and may skip estimation when values are numerically zero.
Previously, toNumber ensured that 0 and invalid values were treated as needing estimation via if (!maxFeeNum || !maxPriorityNum). The new check !maxFeePerGas || !maxPriorityFeePerGas works on the raw inputs, so a string like '0' is truthy and will no longer trigger estimation. This changes behavior for callers passing zero-like values and may result in using a zero max fee/tip instead of estimating. If you want to preserve the old behavior (zero → re-estimate), needsEstimation should be based on numeric-converted values or an explicit zero check rather than the raw inputs.
| let maxFeeNum = toNumber(maxFeePerGas) | ||
| let maxPriorityNum = toNumber(maxPriorityFeePerGas) |
There was a problem hiding this comment.
suggestion (bug_risk): Loss of the previous !maxFeeNum || !maxPriorityNum validation means NaN values are no longer corrected via estimation.
With the removal of this guard, non-numeric maxFeePerGas / maxPriorityFeePerGas can now become NaN via toNumber while needsEstimation is false, so they’ll flow into the comparison logic and downstream fee handling as NaN. To keep the prior safety behavior, add a post-conversion check (e.g. !Number.isFinite(maxFeeNum) or maxFeeNum <= 0, same for priority fee) and fall back to estimates or a safe default when the values are invalid.
| 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 |
There was a problem hiding this comment.
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 needsEstimation is true (at least one of maxFeePerGas / maxPriorityFeePerGas is missing). Please add a test that passes both values in txParams so needsEstimation is false, and verify that:
getFeeEstimatesis not called- The returned
maxFeePerGas/maxPriorityFeePerGasmatch the provided values (used as floors) and still satisfy the EIP-1559 constraint after adjustment.
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 () => {- If the helper used in this file is not
wallet.getGasPricing(txParams, logger)but some other function (e.g. a standalonegetGasPricing(wallet, txParams, logger)), adjust the call accordingly while keeping the assertions identical. - If
maxFeePerGas/maxPriorityFeePerGasin the rest of the suite areBigNumber/BN/numeric types rather than strings, convert the literals and comparisons to match that convention, and adapt the EIP-1559 constraint assertion to use the same big-number utilities.
| } | ||
| } | ||
|
|
||
| pickFeeWithFloor(value: string | number | void, floor: string | number | void) { |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the EIP-1559 fee handling by removing pickFeeWithFloor and doing all floor and estimate logic in a single numeric flow using Math.max and toNumber.
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 pickFeeWithFloor and use numeric comparison
Instead of mixing BigInt, toString, and null-handling in a helper, normalize to numbers once and use Math.max:
// remove this entirely
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
}Inline floor application after estimates:
// 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 hasAnyFeeInput =
maxFeePerGas != null ||
maxPriorityFeePerGas != null
if (!hasAnyFeeInput) {
try {
const { baseFee, priorityFee } = await this.getFeeEstimates()
const baseFeeNum = toNumber(baseFee)
const priorityFeeNum = toNumber(priorityFee)
const maxFeeFloorNum = toNumber(maxFeeFloor)
const maxPriorityFloorNum = toNumber(maxPriorityFloor)
// estimates can rise above floors
const effectiveMaxFeeNum = Math.max(baseFeeNum, maxFeeFloorNum)
const effectiveMaxPriorityNum = Math.max(priorityFeeNum, maxPriorityFloorNum)
maxFeePerGas = effectiveMaxFeeNum.toString()
maxPriorityFeePerGas = effectiveMaxPriorityNum.toString()
} 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
}2. Keep normalization and validation in one place
Now the rest of the logic stays as-is and operates in one numeric domain:
let maxFeeNum = toNumber(maxFeePerGas)
let maxPriorityNum = toNumber(maxPriorityFeePerGas)
if (maxPriorityNum > 0 && (maxFeeNum === 0 || maxFeeNum < maxPriorityNum)) {
logger.warn('maxFeePerGas < maxPriorityFeePerGas or is 0, adjusting maxFeePerGas', {
originalMaxFeePerGas: maxFeePerGas,
maxPriorityFeePerGas: maxPriorityFeePerGas,
adjustedMaxFeePerGas: maxPriorityFeePerGas,
})
maxFeePerGas = maxPriorityFeePerGas
}This keeps the new behavior:
- Instance values are floors.
- Estimates can exceed floors.
- On estimation failure, you fall back to floors.
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.
Looking at How const result = await this.web3.eth.getFeeHistory('0x5', 'latest', [10])
const baseFees = result.baseFeePerGas.map(hex => parseInt(hex, 16))
const rewards = result.reward.map(r => parseInt(r[0], 16)) // 10th percentile
const latestBaseFee = baseFees[baseFees.length - 1]
const minPriorityFee = Math.min(...rewards)
return {
baseFee: Math.floor(latestBaseFee * 1.1),
priorityFee: minPriorityFee
}Why the server consistently gets undervalued results (and locally you don't)
What the fix does and what remains The fix introduces a floor ( The remaining limitation is that
|
|
@copilot locally when running getFeeHistory even within the same minute never produces or sees a maxFeePerGas lower then 50. what does it mean 'it caches' and how often is it being refreshed? |
To be precise: there is no server-side cache of fee estimates in our code. Every call to By "Forno caches" I meant that Forno (as a hosted, load-balanced RPC cluster) may serve a slightly stale The actual root cause was a // Old CeloAdminWallet config:
maxFeePerGas: undefined,
maxPriorityFeePerGas: (2e8).toFixed(0) // "200000000" — 0.2 gwei
// Old normalizeGasPricing:
maxFeePerGas = maxFeePerGas !== undefined ? maxFeePerGas : this.maxFeePerGas
// → maxFeePerGas = undefined (class default was also undefined)
maxPriorityFeePerGas = maxPriorityFeePerGas !== undefined ? maxPriorityFeePerGas : this.maxPriorityFeePerGas
// → maxPriorityFeePerGas = "200000000" (0.2 gwei from config)
// Then:
if (!maxFeeNum || !maxPriorityNum) { // maxFeeNum=0 → enters block
maxFeePerGas = maxFeeNum || baseFee // 0 → uses forno's baseFee ✓
maxPriorityFeePerGas = maxPriorityNum || priorityFee // 200000000 is truthy → NEVER updated ✗
}So the priority fee was always the hardcoded 0.2 gwei, and the base fee was whatever forno returned at that moment. The 30 gwei base fee in the logs was genuine — Celo's base fee does dip to that range during low-activity periods. Because you test locally during higher-activity windows and run single transactions rather than the server's continuous throughput, you've simply not hit those low-base-fee windows. The fix in this PR closes both issues: the priority fee is now updated from forno (with a floor), and the base fee can no longer be lower than the configured floor. |
| const minPriorityFee = Math.min(...rewards) | ||
|
|
||
| return { | ||
| baseFee: Math.floor(latestBaseFee * 1.1), // in wei |
There was a problem hiding this comment.
- acording to the AI review this is the main problem, and I agree. so this multiplier should increase.
- we need to verify the topAdmins is enough
- we need to also look at the faucet, are we giving enough? should we change the contract to look at the block gas price? (we do it in the base faucet we have for superfluid campaign)
- the floor for celo shouldn't be that high 30 gwei should be fine (ie 5 above the minimum of 25)
Description
For some reason forno.celo returns undervalued fee-estimates on the server. this behavior is not able to replicated from my local environment. but the log from the server shows these values being used:
The proposed fix follows changes applied in: https://github.com/GoodDollar/GoodWalletV2/pull/2155
but as far as I can tell if the server caches older fee-estimates anywhere this fix on its own will not change or fix anything.
What has been set as 'floor' hardcoded is at the moment 50 gwei since that is pretty consistently been the base floor since last wendesday.
It would in the short time likely make more transactions succeed but would fail anytime maxFeePerGas is HIGHER then 50 gwei (and this happens on/off throughout the day)
Summary by Sourcery
Adjust gas pricing normalization to treat configured EIP-1559 fee values as floors and prefer higher provider estimates, with Celo-specific fee floors to avoid underpriced transactions.
Bug Fixes:
Enhancements:
Tests: