Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| // Theoretically this should never happen | ||
| gasPrice = Math.round( | ||
| (blockchain.gasPrice || (await this.getNetworkGasPrice(blockchain))) * 1.2, | ||
| (blockchain.gasPrice || (await this.getSmartGasPrice(blockchain))) * 3, |
There was a problem hiding this comment.
why is this hardcoded? I thought we agreed that this should be a multiplier passed by the user and use 3 as a sensible default?
There was a problem hiding this comment.
this is multiplier for exponential on retry, i created a user config for multiplier of regular transaction, ill make this configurable also
| } | ||
|
|
||
| let finalGasPrice; | ||
| if (blockchain.MAXGASPRICE) { |
There was a problem hiding this comment.
why is this logic all over the place? I have a feeling that we are leaking the implementation details a bit?
There was a problem hiding this comment.
its all in same file, want me to move this within inner function?
There was a problem hiding this comment.
let's remove this for now, and have it as a separate PR @MilanKomsa
| } | ||
|
|
||
| let finalGasPrice; | ||
| if (blockchain.MAXGASPRICE) { |
There was a problem hiding this comment.
let's remove this for now, and have it as a separate PR @MilanKomsa
| @@ -208,21 +208,23 @@ export default class BlockchainServiceBase { | |||
| ); | |||
|
|
|||
| if (pendingTx) { | |||
| // If found, increase gas price of pending tx by 20% | |||
| gasPrice = Math.round(Number(pendingTx.gasPrice) * 1.2); | |||
| // If found, increase gas price of pending tx by retryTxGasPriceMultiplier | |||
| gasPrice = Math.round(Number(pendingTx.gasPrice) * blockchain.retryTxGasPriceMultiplier); | |||
| } else { | |||
| // If not found, use default/network gas price increased by 20% | |||
| // If not found, use default/network gas price increased by retryTxGasPriceMultiplier | |||
| // Theoretically this should never happen | |||
| gasPrice = Math.round( | |||
| (blockchain.gasPrice || (await this.getNetworkGasPrice(blockchain))) * 1.2, | |||
| (blockchain.gasPrice || (await this.getSmartGasPrice(blockchain))) * blockchain.retryTxGasPriceMultiplier, | |||
| ); | |||
| } | |||
| } else { | |||
| gasPrice = blockchain.gasPrice || (await this.getNetworkGasPrice(blockchain)); | |||
| gasPrice = blockchain.gasPrice || (await this.getSmartGasPrice(blockchain)); | |||
| } | |||
| } else { | |||
| gasPrice = blockchain.gasPrice || (await this.getNetworkGasPrice(blockchain)); | |||
| } | |||
| gasPrice = blockchain.gasPrice || (await this.getSmartGasPrice(blockchain)); | |||
| }*/ | |||
There was a problem hiding this comment.
With this commented out, do we have the retry logic anywhere else?
Can we remove the previousTxGasPrice and retryTx flags to simplify the code then?
| gasPrice = blockchain.gasPrice || (await this.getNetworkGasPrice(blockchain)); | ||
| } | ||
| gasPrice = blockchain.gasPrice || (await this.getSmartGasPrice(blockchain)); | ||
| }*/ |
There was a problem hiding this comment.
Bug: forceReplaceTxs configuration option is now ignored
The forceReplaceTxs option is still accepted in the input service and included in the blockchain config, but the entire code block that handles this feature (lines 193-223) is now commented out. Users who configure forceReplaceTxs to replace pending transactions with higher gas prices will find this feature silently no longer works.
Additional Locations (1)
services/input-service.js
Outdated
| const retryTxGasPriceMultiplier = | ||
| options.blockchain?.retryTxGasPriceMultiplier ?? | ||
| this.config.blockchain?.retryTxGasPriceMultiplier ?? | ||
| DEFAULT_PARAMETERS.RETRY_TX_GAS_PRICE_MULTIPLIER; // e.g., 1.2 |
There was a problem hiding this comment.
Bug: New retryTxGasPriceMultiplier option is dead code
A new retryTxGasPriceMultiplier configuration option is added along with a default constant, but the only code that would use this value is inside a commented-out block in prepareTransaction. This option has no effect and cannot function until the commented code is re-enabled.
Additional Locations (1)
services/input-service.js
Outdated
|
|
||
| const maxAllowance = | ||
| options.blockchain?.maxAllowance ?? this.config.blockchain?.maxAllowance ?? undefined; | ||
| const bufferPercent = |
There was a problem hiding this comment.
let's name this variable "gasPriceBufferPercent" or similar, so it's obvious what this buffer is for
in the words of late Phil Karlton "There are only two hard things in Computer Science: cache invalidation and naming things."
constants/constants.js
Outdated
| SIMULATE_TXS: false, | ||
| FORCE_REPLACE_TXS: false, | ||
| GAS_LIMIT_MULTIPLIER: 1, | ||
| BUFFER_PERCENT: 10, |
There was a problem hiding this comment.
let's name this variable "GAS_PRICE_BUFFER_PERCENT" or similar, so it's obvious what this buffer is for
in the words of late Phil Karlton "There are only two hard things in Computer Science: cache invalidation and naming things."
| gasPrice = blockchain.gasPrice || (await this.getNetworkGasPrice(blockchain)); | ||
| } | ||
| gasPrice = blockchain.gasPrice || (await this.getSmartGasPrice(blockchain)); | ||
| }*/ |
There was a problem hiding this comment.
Bug: Commented out code should have been deleted
A large block of code (lines 188-225) is commented out instead of being removed. The PR discussion explicitly asks "I guess we remove all this commented out code?" suggesting this commented code was supposed to be deleted entirely, not left in place. Leaving commented-out code in production creates maintenance burden and confusion about what the code is supposed to do.
| gasPrice = blockchain.gasPrice || (await this.getSmartGasPrice(blockchain)); | ||
| }*/ | ||
|
|
||
| const gasPrice = blockchain.gasPrice ?? (await this.getSmartGasPrice(blockchain)); |
There was a problem hiding this comment.
Bug: Retry transaction gas price logic is now broken
The retry transaction gas price feature is broken. In node-blockchain-service.js, retry logic still sets blockchain.retryTx = true and blockchain.previousTxGasPrice when a transaction needs to be retried. However, prepareTransaction now ignores these values entirely and just uses blockchain.gasPrice ?? getSmartGasPrice(). The retryTxGasPriceMultiplier config is added to input-service.js and constants but never used, making retried transactions fail with the same gas price that previously failed.
Additional Locations (1)
4 participants
Note
Adds smart gas pricing via
eth_feeHistorywith optional buffer and overhauls token allowance to approve a threshold; wires new blockchain params.services/blockchain-service/blockchain-service-base.js):getSmartGasPriceusing EIP-1559eth_feeHistorywith fallback to network/default.getFeeHistory,applyGasPriceBuffer,estimateGasPriceFromFeeHistory; usesFEE_HISTORY_BLOCK_COUNT.blockchain.gasPrice ?? getSmartGasPrice(...).needsMoreAllowanceandmaxAllowancePerTransaction.approve(allowanceThreshold)(usesblockchain.maxAllowanceor wallet balance).createKnowledgeCollection.services/input-service.js,constants/constants.js):maxAllowance,gasPriceBufferPercent,retryTxGasPriceMultiplier(default viaDEFAULT_PARAMETERS.RETRY_TX_GAS_PRICE_MULTIPLIER).FEE_HISTORY_BLOCK_COUNTconstant.Written by Cursor Bugbot for commit 8877fdb. This will update automatically on new commits. Configure here.