Skip to content
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.

Sign up for GitHub

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

add frequency check for ema update; cleanup/refactor limit checking #10

Merged
merged 2 commits into from
Nov 10, 2024
Merged

add frequency check for ema update; cleanup/refactor limit checking #10

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cfd067
add frequency check for ema update; cleanup/refactor limit checking
dshiell Nov 10, 2024
dec4be1
fix doUpdate condition
dshiell Nov 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 29 additions & 12 deletions src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ let keyRingId = process.env.KEY_RING_ID!
let keyId = process.env.KEY_ID!
let TX_GASPRICE_LIMIT = BigInt(process.env.TXPRICE_LIMIT!)
let TX_BLOBPRICE_LIMIT = BigInt(process.env.TX_BLOBPRICE_LIMIT!)
let TX_ALPHA = BigInt(process.env.TX_ALPHA ?? "1")
let TX_ALPHA = BigInt(process.env.TX_ALPHA ?? "1") // default alpha is equivalent to 0.01
const EMA_UPDATE_DELTA_SECS = BigInt(process.env.EMA_UPDATE_DELTA_SECS ?? "60") // default update frequency is once a minute

let lastUpdateTime = 0;

kmsProvider.setPath({
projectId: PROJECT_ID,
Expand Down Expand Up @@ -91,27 +94,41 @@ async function feesTooHigh(transactionArgs: TransactionArgs) {
maxFeePerBlobGas = BigInt(transactionArgs.maxFeePerBlobGas);
}

var gasPrice = (maxFeePerGas + maxPriorityFeePerGas);
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;
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;
dshiell marked this conversation as resolved.
Show resolved Hide resolved

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;
}
dshiell marked this conversation as resolved.
Show resolved Hide resolved

// 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;
Comment on lines +97 to +131
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

🛠️ Refactor suggestion

Input validation for negative values is essential

The review comment is correct. Here's why:

  1. The computeLimitEMA function's implementation shows it performs subtraction (price - limit) which could result in negative values with incorrect inputs
  2. The function's comment explicitly states "@brief Assumes that all inputs are non-negative" but doesn't enforce this assumption
  3. 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

}

async function handleEthSignTransaction(transactionArgs: TransactionArgs) {
Expand Down
Loading