-
Notifications
You must be signed in to change notification settings - Fork 24
gas and allowance fixes #266
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.
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
Changes from 15 commits
4735966
4c53b53
cc3809e
cff2c97
abd6cb0
01579bb
5ce76c2
9967168
946473e
3bc339c
f81c83f
5d8aeca
cb08e99
aab74d5
a0fc90e
8877fdb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,9 +7,9 @@ import { createRequire } from 'module'; | |
| import { | ||
| OPERATIONS_STEP_STATUS, | ||
| DEFAULT_GAS_PRICE, | ||
| DEFAULT_GAS_PRICE_WEI, | ||
| ZERO_ADDRESS, | ||
| NEUROWEB_INCENTIVE_TYPE_CHAINS, | ||
| FEE_HISTORY_BLOCK_COUNT, | ||
| } from '../../constants/constants.js'; | ||
| import emptyHooks from '../../util/empty-hooks.js'; | ||
| import { sleepForMilliseconds } from '../utilities.js'; | ||
|
|
@@ -185,10 +185,10 @@ export default class BlockchainServiceBase { | |
| ); | ||
| gasLimit = Math.round(gasLimit * blockchain.gasLimitMultiplier); | ||
|
|
||
| let gasPrice; | ||
| if (blockchain.previousTxGasPrice && blockchain.retryTx) { | ||
| // Increase previous tx gas price by 20% | ||
| gasPrice = Math.round(blockchain.previousTxGasPrice * 1.2); | ||
| // let gasPrice; | ||
| /*if (blockchain.previousTxGasPrice && blockchain.retryTx) { | ||
| // Increase previous tx gas price by retryTxGasPriceMultiplier | ||
| gasPrice = Math.round(blockchain.previousTxGasPrice * blockchain.retryTxGasPriceMultiplier); | ||
zsculac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } else if (blockchain.forceReplaceTxs) { | ||
| // Get the current transaction count (nonce) of the wallet, including pending transactions | ||
| const currentNonce = await web3Instance.eth.getTransactionCount(publicKey, 'pending'); | ||
|
|
@@ -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); | ||
zsculac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } 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, | ||
| ); | ||
| } | ||
zsculac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } 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)); | ||
| }*/ | ||
|
Comment on lines
189
to
225
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this commented out, do we have the retry logic anywhere else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: forceReplaceTxs configuration option is now ignoredThe Additional Locations (1)There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Commented out code should have been deletedA 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. |
||
|
|
||
| const gasPrice = blockchain.gasPrice ?? (await this.getSmartGasPrice(blockchain)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Retry transaction gas price logic is now brokenThe retry transaction gas price feature is broken. In Additional Locations (1) |
||
|
|
||
| if (blockchain.simulateTxs) { | ||
| await web3Instance.eth.call({ | ||
|
|
@@ -317,7 +319,13 @@ export default class BlockchainServiceBase { | |
| } | ||
| } | ||
|
|
||
| async waitForEventFinality(initialReceipt, eventName, expectedEventId, blockchain, confirmations = 1) { | ||
| async waitForEventFinality( | ||
| initialReceipt, | ||
| eventName, | ||
| expectedEventId, | ||
| blockchain, | ||
| confirmations = 1, | ||
| ) { | ||
| await this.ensureBlockchainInfo(blockchain); | ||
| const web3Instance = await this.getWeb3Instance(blockchain); | ||
|
|
||
|
|
@@ -330,7 +338,10 @@ export default class BlockchainServiceBase { | |
| // eslint-disable-next-line no-constant-condition | ||
| while (true) { | ||
| // 1. Wait until the block containing the tx is at the required depth | ||
| while (await web3Instance.eth.getBlockNumber() < receipt.blockNumber + confirmations) { | ||
| while ( | ||
| (await web3Instance.eth.getBlockNumber()) < | ||
| receipt.blockNumber + confirmations | ||
| ) { | ||
| await sleepForMilliseconds(polling); | ||
| } | ||
|
|
||
|
|
@@ -352,7 +363,9 @@ export default class BlockchainServiceBase { | |
|
|
||
| const idMatches = | ||
| expectedEventId == null || | ||
| (eventData && eventData.id != null && eventData.id.toString() === expectedEventId.toString()); | ||
| (eventData && | ||
| eventData.id != null && | ||
| eventData.id.toString() === expectedEventId.toString()); | ||
|
|
||
| if (eventData && idMatches) { | ||
| return { receipt: currentReceipt, eventData }; | ||
|
|
@@ -432,39 +445,49 @@ export default class BlockchainServiceBase { | |
| ); | ||
| } | ||
|
|
||
| async needsMoreAllowance(sender, tokenAmount, blockchain, knowledgeCollectionAddress) { | ||
| const allowance = await this.callContractFunction( | ||
| 'Token', | ||
| 'allowance', | ||
| [sender, knowledgeCollectionAddress], | ||
| blockchain, | ||
| ); | ||
|
|
||
| if (BigInt(allowance) < BigInt(tokenAmount)) return true; | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| async maxAllowancePerTransaction(sender, blockchain) { | ||
| if (blockchain.maxAllowance) { | ||
| return blockchain.maxAllowance; | ||
| } else { | ||
| return await this.callContractFunction('Token', 'balanceOf', [sender], blockchain); | ||
| } | ||
| } | ||
|
|
||
| async increaseKnowledgeCollectionAllowance(sender, tokenAmount, blockchain) { | ||
| const knowledgeCollectionAddress = await this.getContractAddress( | ||
| 'KnowledgeCollection', | ||
| blockchain, | ||
| ); | ||
|
|
||
| const allowance = await this.callContractFunction( | ||
| 'Token', | ||
| 'allowance', | ||
| [sender, knowledgeCollectionAddress], | ||
| const needsMoreAllowance = await this.needsMoreAllowance( | ||
| sender, | ||
| tokenAmount, | ||
| blockchain, | ||
| knowledgeCollectionAddress, | ||
| ); | ||
|
|
||
| const allowanceGap = BigInt(tokenAmount) - BigInt(allowance); | ||
|
|
||
| if (allowanceGap > 0) { | ||
| if (needsMoreAllowance) { | ||
| const allowanceThreshold = await this.maxAllowancePerTransaction(sender, blockchain); | ||
| await this.executeContractFunction( | ||
| 'Token', | ||
| 'increaseAllowance', | ||
| [knowledgeCollectionAddress, allowanceGap], | ||
| 'approve', | ||
| [knowledgeCollectionAddress, allowanceThreshold], | ||
| blockchain, | ||
| ); | ||
|
|
||
| return { | ||
| allowanceIncreased: true, | ||
| allowanceGap, | ||
| }; | ||
| } | ||
zsculac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| return { | ||
| allowanceIncreased: false, | ||
| allowanceGap, | ||
| }; | ||
| } | ||
|
|
||
| // Knowledge assets operations | ||
|
|
@@ -477,19 +500,16 @@ export default class BlockchainServiceBase { | |
| stepHooks = emptyHooks, | ||
| ) { | ||
| const sender = await this.getPublicKey(blockchain); | ||
| let allowanceIncreased = false; | ||
| let allowanceGap = 0; | ||
|
|
||
| try { | ||
| if (requestData?.paymaster && requestData?.paymaster !== ZERO_ADDRESS) { | ||
| // Handle the case when payer is passed | ||
| } else { | ||
| ({ allowanceIncreased, allowanceGap } = | ||
| await this.increaseKnowledgeCollectionAllowance( | ||
| sender, | ||
| requestData.tokenAmount, | ||
| blockchain, | ||
| )); | ||
| await this.increaseKnowledgeCollectionAllowance( | ||
| sender, | ||
| requestData.tokenAmount, | ||
| blockchain, | ||
| ); | ||
| } | ||
|
|
||
| stepHooks.afterHook({ | ||
|
|
@@ -528,9 +548,7 @@ export default class BlockchainServiceBase { | |
|
|
||
| return { knowledgeCollectionId: id, receipt }; | ||
| } catch (error) { | ||
| if (allowanceIncreased) { | ||
| await this.decreaseKnowledgeCollectionAllowance(allowanceGap, blockchain); | ||
| } | ||
| console.error('createKnowledgeCollection failed:', error); | ||
| throw error; | ||
| } | ||
zsculac marked this conversation as resolved.
Show resolved
Hide resolved
zsculac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
@@ -1371,6 +1389,111 @@ export default class BlockchainServiceBase { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Get fee history from the last N blocks using eth_feeHistory RPC call | ||
| * @param {Object} blockchain - Blockchain configuration | ||
| * @param {number} blockCount - Number of blocks to fetch (default: 5) | ||
| * @returns {Promise<Object>} Fee history data with baseFeePerGas and priorityFees arrays | ||
| */ | ||
| async getFeeHistory(blockchain, blockCount = 5) { | ||
| await this.ensureBlockchainInfo(blockchain); | ||
| const web3Instance = await this.getWeb3Instance(blockchain); | ||
|
|
||
| try { | ||
| // eth_feeHistory params: blockCount, newestBlock, rewardPercentiles | ||
| // [50] = median priority fee per block | ||
| const feeHistory = await web3Instance.eth.getFeeHistory(blockCount, 'latest', [50]); | ||
|
|
||
| // Extract median priority fees from each block (reward[blockIndex][percentileIndex]) | ||
| const priorityFees = feeHistory.reward | ||
| ? feeHistory.reward.map((blockRewards) => BigInt(blockRewards[0] || 0)) | ||
| : []; | ||
|
|
||
| return { | ||
| supported: true, | ||
| oldestBlock: parseInt(feeHistory.oldestBlock, 16), | ||
| baseFeePerGas: feeHistory.baseFeePerGas.map((bf) => BigInt(bf)), | ||
| priorityFees, | ||
| }; | ||
| } catch (error) { | ||
| // eth_feeHistory not supported on this network | ||
| return { | ||
| supported: false, | ||
| error: error.message, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Apply buffer percentage to a gas price | ||
| * @param {BigInt} gasPrice - Gas price in wei | ||
| * @param {number} bufferPercent - Buffer percentage to add | ||
| * @returns {BigInt} Gas price with buffer applied | ||
| */ | ||
| applyGasPriceBuffer(gasPrice, bufferPercent) { | ||
| if (!bufferPercent) return gasPrice; | ||
| return (gasPrice * BigInt(100 + Number(bufferPercent))) / 100n; | ||
| } | ||
|
|
||
| /** | ||
| * Estimate safe gas price using eth_feeHistory (EIP-1559 style) | ||
| * Takes max base fee from last N blocks, adds a buffer for volatility, | ||
| * and includes the priority fee (tip) for validator incentive | ||
| * @param {Object} blockchain - Blockchain configuration | ||
| * @returns {Promise<BigInt>} Estimated gas price in wei | ||
| */ | ||
| async estimateGasPriceFromFeeHistory(blockchain) { | ||
| const { bufferPercent } = blockchain; | ||
| const feeHistory = await this.getFeeHistory(blockchain, FEE_HISTORY_BLOCK_COUNT); | ||
|
|
||
| // Fallback to network gas price if feeHistory not supported or empty | ||
| if (!feeHistory.supported) { | ||
| return this.applyGasPriceBuffer( | ||
| BigInt(await this.getNetworkGasPrice(blockchain)), | ||
| bufferPercent, | ||
| ); | ||
| } | ||
|
|
||
| const baseFees = Array.from(feeHistory.baseFeePerGas); | ||
| const priorityFees = Array.from(feeHistory.priorityFees); | ||
|
|
||
| if (baseFees.length === 0 || priorityFees.length === 0) { | ||
| return this.applyGasPriceBuffer( | ||
| BigInt(await this.getNetworkGasPrice(blockchain)), | ||
| bufferPercent, | ||
| ); | ||
| } | ||
|
|
||
| // Find max base fee and priority fee from recent blocks | ||
| const maxBaseFee = baseFees.reduce((max, bf) => (bf > max ? bf : max), 0n); | ||
| const maxPriorityFee = priorityFees.reduce((max, pf) => (pf > max ? pf : max), 0n); | ||
|
|
||
| return this.applyGasPriceBuffer(maxBaseFee + maxPriorityFee, bufferPercent); | ||
| } | ||
|
|
||
| /** | ||
| * Get gas price with EIP-1559 estimation (with fallback) | ||
| * Tries eth_feeHistory first, falls back to legacy methods | ||
| * @param {Object} blockchain - Blockchain configuration | ||
| * @returns {Promise<string>} Gas price in wei (as string for web3 compatibility) | ||
| */ | ||
| async getSmartGasPrice(blockchain) { | ||
| try { | ||
| const estimatedPrice = await this.estimateGasPriceFromFeeHistory(blockchain); | ||
| return estimatedPrice.toString(); | ||
| } catch (eip1559Error) { | ||
| try { | ||
| return await this.getNetworkGasPrice(blockchain); | ||
| } catch (fallbackError) { | ||
| throw new Error( | ||
| `All gas price estimation methods failed. ` + | ||
| `EIP-1559: ${eip1559Error?.message || 'N/A'}. ` + | ||
| `Fallback: ${fallbackError.message}`, | ||
| ); | ||
| } | ||
| } | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| async getWalletBalances(blockchain) { | ||
| await this.ensureBlockchainInfo(blockchain); | ||
| const web3Instance = await this.getWeb3Instance(blockchain); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -192,6 +192,15 @@ export default class InputService { | |
| BLOCKCHAINS[environment][name]?.gasPriceOracleLink ?? | ||
| undefined; | ||
|
|
||
| const maxAllowance = | ||
| options.blockchain?.maxAllowance ?? this.config.blockchain?.maxAllowance ?? undefined; | ||
| const bufferPercent = | ||
|
||
| options.blockchain?.bufferPercent ?? this.config.blockchain?.bufferPercent ?? undefined; | ||
| const retryTxGasPriceMultiplier = | ||
| options.blockchain?.retryTxGasPriceMultiplier ?? | ||
| this.config.blockchain?.retryTxGasPriceMultiplier ?? | ||
| DEFAULT_PARAMETERS.RETRY_TX_GAS_PRICE_MULTIPLIER; // e.g., 1.2 | ||
|
|
||
| const blockchainConfig = { | ||
| name, | ||
| rpc, | ||
|
|
@@ -205,6 +214,9 @@ export default class InputService { | |
| simulateTxs, | ||
| forceReplaceTxs, | ||
| gasPriceOracleLink, | ||
| maxAllowance, | ||
| bufferPercent, | ||
| retryTxGasPriceMultiplier, | ||
| }; | ||
|
|
||
| if (name && name.startsWith('otp')) { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.