-
Notifications
You must be signed in to change notification settings - Fork 25
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 4 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
zsculac marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -186,9 +186,9 @@ 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); | ||
| /*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. |
||
|
|
||
| gasPrice = await this.getSmartGasPrice(blockchain); | ||
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| if (blockchain.simulateTxs) { | ||
| await web3Instance.eth.call({ | ||
|
|
@@ -238,7 +240,7 @@ export default class BlockchainServiceBase { | |
| from: publicKey, | ||
| to: contractInstance.options.address, | ||
| data: encodedABI, | ||
| gasPrice, | ||
| gasPrice: gasPrice, | ||
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| gas: gasLimit, | ||
| }; | ||
| } | ||
|
|
@@ -282,7 +284,7 @@ export default class BlockchainServiceBase { | |
| while ( | ||
| !currentReceipt && | ||
| Date.now() - reminingStartTime < | ||
| blockchain.transactionReminingMaxWaitTime | ||
| blockchain.transactionReminingMaxWaitTime | ||
| ) { | ||
| await sleepForMilliseconds( | ||
| blockchain.transactionReminingPollingInterval, | ||
|
|
@@ -432,11 +434,7 @@ export default class BlockchainServiceBase { | |
| ); | ||
| } | ||
|
|
||
| async increaseKnowledgeCollectionAllowance(sender, tokenAmount, blockchain) { | ||
| const knowledgeCollectionAddress = await this.getContractAddress( | ||
| 'KnowledgeCollection', | ||
| blockchain, | ||
| ); | ||
| async needsMoreAllowance(sender, tokenAmount, blockchain, knowledgeCollectionAddress) { | ||
|
|
||
| const allowance = await this.callContractFunction( | ||
| 'Token', | ||
|
|
@@ -445,25 +443,51 @@ export default class BlockchainServiceBase { | |
| blockchain, | ||
| ); | ||
|
|
||
| const allowanceGap = BigInt(tokenAmount) - BigInt(allowance); | ||
| if (BigInt(allowance) < BigInt(tokenAmount)) | ||
| return true; | ||
| else | ||
| return false; | ||
| } | ||
|
|
||
| if (allowanceGap > 0) { | ||
| await this.executeContractFunction( | ||
| async maxAllowancePerTransaction(sender, blockchain) { | ||
| if (blockchain.maxAllowance) { | ||
| return blockchain.maxAllowance; | ||
| } else { | ||
| return await this.callContractFunction( | ||
| 'Token', | ||
| 'increaseAllowance', | ||
| [knowledgeCollectionAddress, allowanceGap], | ||
| 'balanceOf', | ||
| [sender], | ||
| blockchain, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| async increaseKnowledgeCollectionAllowance(sender, tokenAmount, blockchain) { | ||
| const knowledgeCollectionAddress = await this.getContractAddress( | ||
| 'KnowledgeCollection', | ||
| blockchain, | ||
| ); | ||
|
|
||
| const needsMoreAllowance = await this.needsMoreAllowance(sender, tokenAmount, blockchain, knowledgeCollectionAddress); | ||
|
|
||
| let allowanceThreshold = await this.maxAllowancePerTransaction(sender, blockchain); | ||
|
|
||
| if (needsMoreAllowance) { | ||
| await this.executeContractFunction( | ||
| 'Token', | ||
| 'approve', | ||
| [knowledgeCollectionAddress, allowanceThreshold], | ||
| blockchain, | ||
| ); | ||
| return { | ||
| allowanceIncreased: true, | ||
| allowanceGap, | ||
| allowanceCurrent: allowanceThreshold, | ||
| }; | ||
| } | ||
zsculac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| return { | ||
| allowanceIncreased: false, | ||
| allowanceGap, | ||
| allowanceCurrent: allowanceThreshold, | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -478,13 +502,13 @@ export default class BlockchainServiceBase { | |
| ) { | ||
| const sender = await this.getPublicKey(blockchain); | ||
| let allowanceIncreased = false; | ||
| let allowanceGap = 0; | ||
| let allowanceCurrent = 0; | ||
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| try { | ||
| if (requestData?.paymaster && requestData?.paymaster !== ZERO_ADDRESS) { | ||
| // Handle the case when payer is passed | ||
| } else { | ||
| ({ allowanceIncreased, allowanceGap } = | ||
| ({ allowanceIncreased, allowanceCurrent } = | ||
| await this.increaseKnowledgeCollectionAllowance( | ||
| sender, | ||
| requestData.tokenAmount, | ||
|
|
@@ -528,9 +552,11 @@ export default class BlockchainServiceBase { | |
|
|
||
| return { knowledgeCollectionId: id, receipt }; | ||
| } catch (error) { | ||
| if (allowanceIncreased) { | ||
| await this.decreaseKnowledgeCollectionAllowance(allowanceGap, blockchain); | ||
| /*if (allowanceIncreased) { | ||
| await this.decreaseKnowledgeCollectionAllowance(allowanceCurrent, blockchain); | ||
cursor[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| throw error;*/ | ||
| 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
|
||
| } | ||
|
|
@@ -1043,7 +1069,7 @@ export default class BlockchainServiceBase { | |
|
|
||
| if ( | ||
| this[blockchain.name].contractAddresses[blockchain.hubContract][ | ||
| 'ParanetIncentivesPoolStorage' | ||
| 'ParanetIncentivesPoolStorage' | ||
| ] !== contractAddress | ||
| ) { | ||
| this[blockchain.name].contractAddresses[blockchain.hubContract][ | ||
|
|
@@ -1055,7 +1081,7 @@ export default class BlockchainServiceBase { | |
| ] = await new web3Instance.eth.Contract( | ||
| this.abis['ParanetIncentivesPoolStorage'], | ||
| this[blockchain.name].contractAddresses[blockchain.hubContract][ | ||
| 'ParanetIncentivesPoolStorage' | ||
| 'ParanetIncentivesPoolStorage' | ||
| ], | ||
| { from: blockchain.publicKey }, | ||
| ); | ||
|
|
@@ -1090,7 +1116,7 @@ export default class BlockchainServiceBase { | |
|
|
||
| if ( | ||
| this[blockchain.name].contractAddresses[blockchain.hubContract][ | ||
| 'ParanetIncentivesPool' | ||
| 'ParanetIncentivesPool' | ||
| ] !== contractAddress | ||
| ) { | ||
| this[blockchain.name].contractAddresses[blockchain.hubContract][ | ||
|
|
@@ -1101,7 +1127,7 @@ export default class BlockchainServiceBase { | |
| await new web3Instance.eth.Contract( | ||
| this.abis['ParanetIncentivesPool'], | ||
| this[blockchain.name].contractAddresses[blockchain.hubContract][ | ||
| 'ParanetIncentivesPool' | ||
| 'ParanetIncentivesPool' | ||
| ], | ||
| { from: blockchain.publicKey }, | ||
| ); | ||
|
|
@@ -1371,6 +1397,96 @@ 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 array | ||
| */ | ||
| async getFeeHistory(blockchain, blockCount = 5) { | ||
| await this.ensureBlockchainInfo(blockchain); | ||
| const web3Instance = await this.getWeb3Instance(blockchain); | ||
|
|
||
| try { | ||
| const latestBlock = await web3Instance.eth.getBlockNumber(); | ||
cursor[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| // eth_feeHistory params: blockCount (hex), newestBlock (hex), rewardPercentiles | ||
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| const feeHistory = await web3Instance.eth.getFeeHistory(blockCount, 'latest', []); | ||
| console.log('feeHistory', feeHistory); | ||
cursor[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| return { | ||
| supported: true, | ||
| oldestBlock: parseInt(feeHistory.oldestBlock, 16), | ||
| baseFeePerGas: feeHistory.baseFeePerGas.map(bf => BigInt(bf)) | ||
| }; | ||
|
|
||
| } catch (error) { | ||
| // eth_feeHistory not supported on this network | ||
| return { | ||
| supported: false, | ||
| error: error.message, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Estimate safe gas price using eth_feeHistory (EIP-1559 style) | ||
| * Takes max base fee from last N blocks and adds a buffer | ||
| * @param {Object} blockchain - Blockchain configuration | ||
| * @param {Object} options - Options | ||
| * @param {number} options.blockCount - Number of blocks to analyze (default: 5) | ||
| * @param {number} options.bufferPercent - Buffer percentage to add (default: 10) | ||
| * @returns {Promise<BigInt>} Estimated gas price in wei | ||
| */ | ||
| async estimateGasPriceFromFeeHistory(blockchain, options = {}) { | ||
| const { blockCount = 5, bufferPercent = 10 } = options; | ||
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| const feeHistory = await this.getFeeHistory(blockchain, blockCount); | ||
|
|
||
| if (!feeHistory.supported) { | ||
| // Fallback to existing method if eth_feeHistory not supported | ||
| console.warn(`eth_feeHistory not supported: ${feeHistory.error}. Using fallback.`); | ||
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| return BigInt(await this.getNetworkGasPrice(blockchain)); | ||
| } | ||
|
|
||
| // Get base fees (exclude last element - it's for the next block) | ||
| const baseFees = feeHistory.baseFeePerGas.slice(0, -1); | ||
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| if (baseFees.length === 0) { | ||
| return BigInt(await this.getNetworkGasPrice(blockchain)); | ||
| } | ||
|
|
||
| // Find max base fee from recent blocks | ||
| let maxBaseFee = baseFees.reduce((max, bf) => bf > max ? bf : max, 0n); | ||
|
|
||
| // Add buffer (e.g., 20% = multiply by 120, divide by 100) | ||
| let safeGasPrice = (maxBaseFee * BigInt(100 + bufferPercent)) / 100n; | ||
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| if(this.isGnosis(blockchain.name)) { | ||
| safeGasPrice = safeGasPrice + 1n; | ||
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| return safeGasPrice; | ||
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /** | ||
| * 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) { | ||
| // Only use EIP-1559 estimation for chains that support it | ||
|
|
||
| try { | ||
| const estimatedPrice = await this.estimateGasPriceFromFeeHistory(blockchain, blockchain.bufferPercent ? { bufferPercent: blockchain.bufferPercent } : {}); | ||
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| console.log('estimatedPrice', estimatedPrice); | ||
zsculac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| return estimatedPrice.toString(); | ||
| } catch (error) { | ||
| console.warn(`EIP-1559 gas estimation failed: ${error.message}. Using fallback.`); | ||
| } | ||
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,17 @@ 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 ?? | ||
| DEFAULT_PARAMETERS.BUFFER_PERCENT; // e.g., 50 | ||
| 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 +216,9 @@ export default class InputService { | |
| simulateTxs, | ||
| forceReplaceTxs, | ||
| gasPriceOracleLink, | ||
| maxAllowance, | ||
| bufferPercent, | ||
| retryTxGasPriceMultiplier | ||
| }; | ||
|
|
||
| if (name && name.startsWith('otp')) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."