split alpha for increasing and decreasing limits #11
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the handling of transaction parameters and limits within 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: 1
🧹 Outside diff range and nitpick comments (3)
src/server.ts (3)
8-8: LGTM! Consider adding input validation for negative values.The implementation of separate alpha values with appropriate defaults (0.01 for increase, 0.1 for decrease) aligns well with the PR objective of faster downward adjustments. The
minBigInthelper correctly caps values at 100.Consider adding validation for negative values:
-const minBigInt = (a: bigint, b: bigint): bigint => (a < b ? a : b); +const minBigInt = (a: bigint, b: bigint): bigint => { + if (a < 0n || b < 0n) throw new Error("Alpha values must be non-negative"); + return a < b ? a : b; +};Also applies to: 23-24
77-81: LGTM! Consider updating the function documentation.The implementation correctly applies different alpha values based on whether the price is increasing or decreasing, which aligns with the PR objective.
Update the JSDoc to reflect the removal of the alpha parameter and document the dual-alpha behavior:
/** * Computes a new gas limit using exponential moving average. * @param price Current transaction price * @param limit Current gas limit + * @remarks Uses TX_ALPHA_INCREASE when price > limit, TX_ALPHA_DECREASE otherwise * @returns New gas limit * @brief Assumes that all inputs are non-negative */
111-118: LGTM! Consider adding overflow protection.The limit updates are correctly implemented with appropriate logging. However, since we're dealing with bigint arithmetic, it would be prudent to add overflow protection.
Consider wrapping the computations in a try-catch to handle potential arithmetic overflow:
- const newGasLimit = computeLimitEMA(gasPrice, TX_GASPRICE_LIMIT); - console.log('Updating TX_GASPRICE_LIMIT: %d -> %d', TX_GASPRICE_LIMIT, newGasLimit); - TX_GASPRICE_LIMIT = newGasLimit; + try { + const newGasLimit = computeLimitEMA(gasPrice, TX_GASPRICE_LIMIT); + console.log('Updating TX_GASPRICE_LIMIT: %d -> %d', TX_GASPRICE_LIMIT, newGasLimit); + TX_GASPRICE_LIMIT = newGasLimit; + } catch (error) { + console.error('Failed to update gas limit:', error); + // Continue with existing limit + }Apply similar protection to the blob limit update.
Introduces a different alpha for EMA gas/blob price limit updates depending on whether the limit is increasing or decreasing.
Usually when we increase the price limits the batcher retires for long periods of time which can bring up the limit. Once high it could take many batcher cycles for the price limit to decrease because if the limit is high already the batcher will like do only a single price limit update per batch interval.
This change sets the default so that we adjust downwards at 10x the rate of increases by default. The alphas are configurable and should result in the same behavior if they are set equal. Setting to 0 effectively disables EMA and uses the initial price limits as static limits. An alpha of 100 would always move the limit immediately to the current gas/blob price.
Summary by CodeRabbit
New Features
Bug Fixes