add frequency check for ema update; cleanup/refactor limit checking #10
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
src/server.ts (4)
22-25: Consider increasing the default TX_ALPHA value.The default TX_ALPHA value of 1 (equivalent to 0.01) makes the EMA very slow to react to price changes. Consider increasing this value to make the system more responsive to market conditions while still maintaining stability.
-let TX_ALPHA = BigInt(process.env.TX_ALPHA ?? "1") // default alpha is equivalent to 0.01 +let TX_ALPHA = BigInt(process.env.TX_ALPHA ?? "10") // default alpha is equivalent to 0.1
117-131: Improve error logging for fee rejections.The current error logging could be more informative for debugging purposes.
if (gasPrice > TX_GASPRICE_LIMIT) { - console.error('Tx fees too high: %d > %d', gasPrice, TX_GASPRICE_LIMIT); + console.error('Transaction rejected: Gas price (%d) exceeds limit (%d) by %d', + gasPrice, TX_GASPRICE_LIMIT, gasPrice - TX_GASPRICE_LIMIT); rejectTxn = true; } if (transactionArgs.blobVersionedHashes && transactionArgs.blobVersionedHashes.length > 0) { if (maxFeePerBlobGas > TX_BLOBPRICE_LIMIT) { - console.error('Blob fees too high: %d > %d', maxFeePerBlobGas, TX_BLOBPRICE_LIMIT ); + console.error('Transaction rejected: Blob fee (%d) exceeds limit (%d) by %d', + maxFeePerBlobGas, TX_BLOBPRICE_LIMIT, maxFeePerBlobGas - TX_BLOBPRICE_LIMIT); rejectTxn = true; } }
Line range hint
67-73: Add input validation to computeLimitEMA function.The function assumes non-negative inputs but doesn't validate them. This could lead to unexpected behavior with negative values.
function computeLimitEMA(price: bigint, limit: bigint, alpha: bigint) { + if (price < 0 || limit < 0 || alpha < 0) { + throw new Error('computeLimitEMA: Inputs must be non-negative'); + } + if (alpha > 100n) { + throw new Error('computeLimitEMA: Alpha must be <= 100'); + } return (price - limit) * alpha / BigInt(100) + limit; }
97-131: Consider adding metrics for fee rejections.To better understand the system's behavior in production, consider adding metrics for:
- Number of transactions rejected due to high fees
- Current limits vs actual fees
- Frequency of limit updates
Would you like me to provide an example implementation using a metrics library?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/server.ts (4)
22-22: Fix misleading comment about TX_ALPHA value.The comment states that alpha of 1 is equivalent to 0.01, but this is incorrect. With the current implementation, an alpha of 1 means 0.01 (1/100) weight in the EMA calculation.
-let TX_ALPHA = BigInt(process.env.TX_ALPHA ?? "1") // default alpha is equivalent to 0.01 +let TX_ALPHA = BigInt(process.env.TX_ALPHA ?? "1") // default alpha of 1 means 0.01 (1/100) weight in EMA
22-25: Consider improving type safety and immutability.The current implementation uses mutable globals with implicit typing. Consider:
- Using TypeScript's const assertions
- Adding explicit type annotations
- Making variables readonly where possible
-let TX_ALPHA = BigInt(process.env.TX_ALPHA ?? "1") -const EMA_UPDATE_DELTA_SECS = BigInt(process.env.EMA_UPDATE_DELTA_SECS ?? "60") -let lastUpdateTime = 0; +const TX_ALPHA: bigint = BigInt(process.env.TX_ALPHA ?? "1") as const; +const EMA_UPDATE_DELTA_SECS: bigint = BigInt(process.env.EMA_UPDATE_DELTA_SECS ?? "60") as const; +let lastUpdateTime: number = 0;
97-99: Consider using monotonic time for elapsed time calculation.Using
Date.now()for timing can be problematic if the system clock is adjusted. Consider usingprocess.hrtime.bigint()for more reliable timing.-const now = Math.floor(Date.now() / 1000); -const elapsed_secs = now - lastUpdateTime; +const now = process.hrtime.bigint() / BigInt(1e9); +const elapsed_secs = Number(now - BigInt(lastUpdateTime));
117-120: Standardize error logging format.The error logging format is inconsistent between gas and blob price checks. Consider using structured logging with consistent fields.
-console.error('Tx fees too high: %d > %d', gasPrice, TX_GASPRICE_LIMIT); -console.error('Blob fees too high: %d > %d', maxFeePerBlobGas, TX_BLOBPRICE_LIMIT); +console.error('Fee limit exceeded', { + type: 'gas', + actual: gasPrice.toString(), + limit: TX_GASPRICE_LIMIT.toString() +}); +console.error('Fee limit exceeded', { + type: 'blob', + actual: maxFeePerBlobGas.toString(), + limit: TX_BLOBPRICE_LIMIT.toString() +});Also applies to: 123-127
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/server.ts(2 hunks)
🔇 Additional comments (1)
src/server.ts (1)
103-115: The past review comment about thread safety is still valid.
The concurrent updates to lastUpdateTime and limits could lead to race conditions. The suggested mutex solution from the past review should be implemented.
| const now = Math.floor(Date.now() / 1000); | ||
| const elapsed_secs = now - lastUpdateTime; | ||
| const doUpdate = (elapsed_secs >= EMA_UPDATE_DELTA_SECS); | ||
| const gasPrice = (maxFeePerGas + maxPriorityFeePerGas); | ||
| let rejectTxn = false; | ||
|
|
||
| if (doUpdate) { | ||
| lastUpdateTime = now; | ||
|
|
||
| // update gas limit | ||
| const newGasLimit = computeLimitEMA(gasPrice, TX_GASPRICE_LIMIT, TX_ALPHA); | ||
| console.log('Updating TX_GASPRICE_LIMIT: %d -> %d', TX_GASPRICE_LIMIT, newGasLimit); | ||
| TX_GASPRICE_LIMIT = newGasLimit; | ||
|
|
||
| // update blob limit | ||
| const newBlobLimit = computeLimitEMA(maxFeePerBlobGas, TX_BLOBPRICE_LIMIT, TX_ALPHA); | ||
| console.log('Updating TX_BLOBPRICE_LIMIT: %d -> %d', TX_BLOBPRICE_LIMIT, newBlobLimit); | ||
| TX_BLOBPRICE_LIMIT = newBlobLimit; | ||
| } | ||
|
|
||
| // check gas price limit | ||
| if (gasPrice > TX_GASPRICE_LIMIT) { | ||
| console.error('Tx fees too high: %d > %d', gasPrice, TX_GASPRICE_LIMIT); | ||
| return true; | ||
| rejectTxn = true; | ||
| } | ||
|
|
||
| // check blob price limits if applicable | ||
| if (transactionArgs.blobVersionedHashes && transactionArgs.blobVersionedHashes.length > 0) { | ||
| const newBlobLimit = computeLimitEMA(maxFeePerBlobGas, TX_BLOBPRICE_LIMIT, TX_ALPHA); | ||
| console.log('Updating TX_BLOBPRICE_LIMIT: %d -> %d', TX_BLOBPRICE_LIMIT, newBlobLimit); | ||
| TX_BLOBPRICE_LIMIT = newBlobLimit; | ||
|
|
||
| if (maxFeePerBlobGas > TX_BLOBPRICE_LIMIT) { | ||
| console.error('Blob fees too high: %d > %d', maxFeePerBlobGas, TX_BLOBPRICE_LIMIT ); | ||
| return true; | ||
| rejectTxn = true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| return rejectTxn; |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Input validation for negative values is essential
The review comment is correct. Here's why:
- The
computeLimitEMAfunction's implementation shows it performs subtraction (price - limit) which could result in negative values with incorrect inputs - The function's comment explicitly states "@brief Assumes that all inputs are non-negative" but doesn't enforce this assumption
- Using BigInt arithmetic with negative values could lead to unexpected results in the EMA calculation
The suggested input validation in the review comment should be implemented to prevent potential issues with negative fee values.
🔗 Analysis chain
Add input validation for negative values.
The function assumes all inputs are non-negative but doesn't validate this assumption. Consider adding checks for negative values.
+ // Validate non-negative inputs
+ if (maxFeePerGas < 0 || maxPriorityFeePerGas < 0 || maxFeePerBlobGas < 0) {
+ console.error('Negative fee values are not allowed');
+ return true;
+ }Additionally, verify that the EMA calculation in computeLimitEMA handles negative price-limit differences correctly.
Let me search for the implementation of computeLimitEMA to analyze how it handles negative values.
Let me try a different approach to find the implementation of computeLimitEMA.
Let me try one final search with a broader pattern to locate the function definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests or usage of computeLimitEMA with negative values
rg -l "computeLimitEMA.*-" || echo "No tests found for negative values"
Length of output: 107
Script:
#!/bin/bash
# Find implementation of computeLimitEMA
ast-grep --pattern 'function computeLimitEMA'
Length of output: 47
Script:
#!/bin/bash
# Search for computeLimitEMA implementation using ripgrep
rg -A 5 "computeLimitEMA.*="
Length of output: 30
Script:
#!/bin/bash
# Search for any mention of computeLimitEMA in the codebase
rg "computeLimitEMA" -C 5
Length of output: 1373
Summary by CodeRabbit
New Features
Bug Fixes