Fetch gas prices from chain if empty #113
Conversation
WalkthroughThis update bumps the package version from "0.2.31" to "0.2.32" in package.json. Additionally, a new asynchronous method Changes
Sequence Diagram(s)sequenceDiagram
participant TxAPI
participant RESTClient
participant OpchildAPI as opchild API
participant MoveAPI as move API
TxAPI->>RESTClient: Call gasPrices()
RESTClient->>OpchildAPI: Request gas prices
OpchildAPI-->>RESTClient: Return gas price or error
alt Opchild API success
RESTClient-->>TxAPI: Return gas price string
else Opchild API failure
RESTClient->>MoveAPI: Request gas prices
MoveAPI-->>RESTClient: Return gas price details
RESTClient-->>TxAPI: Return formatted coin string
end
Poem
✨ Finishing Touches
🪧 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 (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
![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
🧹 Nitpick comments (3)
src/client/rest/api/TxAPI.ts (1)
302-304: Defensive improvement: Add fallback for undefined gas prices.Your new code ensures gas prices are fetched from the chain if none are configured, which is great. However, there's a potential edge case where
this.rest.gasPrices()might returnundefinedbased on its implementation, and this case isn't explicitly handled here.Consider adding additional fallback logic:
if (!this.rest.config.gasPrices) { - this.rest.config.gasPrices = await this.rest.gasPrices() + const fetchedGasPrices = await this.rest.gasPrices() + this.rest.config.gasPrices = fetchedGasPrices || '0uinit' // Provide a default value if fetch fails }src/client/rest/RESTClient.ts (2)
166-181: Add JSDoc documentation and error logging.The implementation of the
gasPricesmethod is well-structured with proper fallback logic. However, it could benefit from JSDoc documentation and error logging to assist with debugging.Add documentation and error logging:
+/** + * Retrieves gas prices from the chain. + * First attempts to fetch from opchild parameters, then falls back to move parameters. + * @returns {Promise<Coins.Input | undefined>} The gas prices as a string or undefined if both fetches fail + */ public async gasPrices(): Promise<Coins.Input | undefined> { try { const opchildParams = await this.opchild.parameters() return opchildParams.min_gas_prices.toString() || '0uinit' - } catch { + } catch (error) { + console.debug('Failed to fetch gas prices from opchild API, falling back to move API', error) try { const moveParams = await this.move.parameters() return new Coin( moveParams.base_denom, moveParams.base_min_gas_price ).toString() - } catch { + } catch (error) { + console.debug('Failed to fetch gas prices from move API', error) return undefined } } }
166-181: Consider implementing a caching mechanism.The current implementation fetches gas prices every time they're needed. Given that gas prices don't change frequently, you might want to implement a simple caching mechanism with an expiration period to reduce API calls.
Here's a potential implementation:
+private gasPricesCache: { + value: Coins.Input | undefined; + timestamp: number; +} | null = null; + +private readonly CACHE_EXPIRY_MS = 60000; // 1 minute cache + public async gasPrices(): Promise<Coins.Input | undefined> { + // Return cached value if it exists and hasn't expired + if ( + this.gasPricesCache && + Date.now() - this.gasPricesCache.timestamp < this.CACHE_EXPIRY_MS + ) { + return this.gasPricesCache.value; + } + try { const opchildParams = await this.opchild.parameters() - return opchildParams.min_gas_prices.toString() || '0uinit' + const result = opchildParams.min_gas_prices.toString() || '0uinit'; + // Update cache + this.gasPricesCache = { + value: result, + timestamp: Date.now() + }; + return result; } catch { try { const moveParams = await this.move.parameters() - return new Coin( + const result = new Coin( moveParams.base_denom, moveParams.base_min_gas_price ).toString() + // Update cache + this.gasPricesCache = { + value: result, + timestamp: Date.now() + }; + return result; } catch { + // Update cache with undefined to prevent repeated failed calls + this.gasPricesCache = { + value: undefined, + timestamp: Date.now() + }; return undefined } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(1 hunks)src/client/rest/RESTClient.ts(2 hunks)src/client/rest/api/TxAPI.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🔇 Additional comments (1)
src/client/rest/RESTClient.ts (1)
35-35: LGTM: Import addition for Coin.The addition of the Coin import is appropriate for the new functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/rest/RESTClient.ts (1)
166-181:⚠️ Potential issueGas Price Validation Required
The current implementation doesn't validate that the retrieved gas prices are valid before returning them. This could lead to invalid fee estimations downstream.
Consider implementing these improvements:
- Validate that the values from both APIs adhere to expected coin format
- Ensure numeric components are valid and denominations are allowed values
- Add proper validation before returning values
public async gasPrices(): Promise<Coins.Input | undefined> { try { const opchildParams = await this.opchild.parameters() - return opchildParams.min_gas_prices.toArray()[0]?.toString() ?? '0uinit' + const gasPrice = opchildParams.min_gas_prices.toArray()[0] + // Validate gas price before returning + if (gasPrice && this.isValidCoin(gasPrice)) { + return gasPrice.toString() + } + return '0uinit' } catch { try { const moveParams = await this.move.parameters() - return new Coin( + const gasPrice = new Coin( moveParams.base_denom, moveParams.base_min_gas_price - ).toString() + ) + // Validate gas price before returning + if (this.isValidCoin(gasPrice)) { + return gasPrice.toString() + } + return '0uinit' } catch { return undefined } } } // Add a validation helper method private isValidCoin(coin: Coin): boolean { // Implement validation logic: // 1. Check if amount is a valid number and > 0 // 2. Verify denomination is in the allowed list // 3. Ensure the format meets expected standards return coin && typeof coin.amount === 'string' && /^\d+$/.test(coin.amount) && typeof coin.denom === 'string' && coin.denom.length > 0; }
🧹 Nitpick comments (2)
src/client/rest/RESTClient.ts (2)
166-181: Consider adding logging for error casesThe implementation silently catches errors without logging them, which could make troubleshooting issues in production difficult.
public async gasPrices(): Promise<Coins.Input | undefined> { try { const opchildParams = await this.opchild.parameters() return opchildParams.min_gas_prices.toArray()[0]?.toString() ?? '0uinit' - } catch { + } catch (error) { + console.debug('Failed to fetch gas prices from opchild API:', error) try { const moveParams = await this.move.parameters() return new Coin( moveParams.base_denom, moveParams.base_min_gas_price ).toString() - } catch { + } catch (error) { + console.debug('Failed to fetch gas prices from move API:', error) return undefined } } }
166-181: Consider implementing a retry mechanism with backoffNetwork requests to fetch gas prices might fail temporarily. Adding a retry mechanism would improve reliability.
You could implement a simple retry mechanism with exponential backoff for both API calls:
+ private async retryWithBackoff<T>( + fn: () => Promise<T>, + maxRetries = 3, + initialDelay = 300 + ): Promise<T> { + let retries = 0; + let delay = initialDelay; + + while (retries < maxRetries) { + try { + return await fn(); + } catch (error) { + retries++; + if (retries >= maxRetries) throw error; + + await new Promise(resolve => setTimeout(resolve, delay)); + delay *= 2; // Exponential backoff + } + } + + throw new Error('Max retries reached'); + } public async gasPrices(): Promise<Coins.Input | undefined> { try { - const opchildParams = await this.opchild.parameters() + const opchildParams = await this.retryWithBackoff(() => this.opchild.parameters()) return opchildParams.min_gas_prices.toArray()[0]?.toString() ?? '0uinit' } catch (error) { try { - const moveParams = await this.move.parameters() + const moveParams = await this.retryWithBackoff(() => this.move.parameters()) return new Coin( moveParams.base_denom, moveParams.base_min_gas_price ).toString() } catch { return undefined } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/rest/RESTClient.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
src/client/rest/RESTClient.ts (3)
35-35: Added Coin import to support new gas price fetching functionality.The updated import includes
Coinclass which is now used in the newgasPricesmethod to construct a coin object with denomination and amount.
173-176:✅ Verification successful
Verify that moveParams.base_denom and moveParams.base_min_gas_price are valid
The code assumes that these parameters will always be available and in the expected format.
🏁 Script executed:
#!/bin/bash # Check how move.parameters() is structured and verify the fields we're accessing echo "Checking the move.parameters() structure:" rg -A 10 -B 10 "parameters\(\)" --glob="**/MoveAPI.ts" echo "Searching for usages of base_denom and base_min_gas_price:" rg "base_denom|base_min_gas_price" --glob="*.ts" | grep -v "node_modules"Length of output: 4970
Validation of moveParams Fields is Confirmed
The investigation confirms that both
base_denomandbase_min_gas_priceare defined as string fields insrc/core/move/MoveParams.tsand are consistently referenced elsewhere (e.g., in the REST client and other modules). Their use in constructing a newCoinappears valid under the current data contracts. No additional changes are required unless you plan to add runtime validation for extra safety.
169-169: 🛠️ Refactor suggestionAdd fallback mechanism for empty min_gas_prices array
The current implementation might return
undefinedif themin_gas_pricesarray is empty, which could cause issues downstream.- return opchildParams.min_gas_prices.toArray()[0]?.toString() ?? '0uinit' + const gasPricesArray = opchildParams.min_gas_prices.toArray(); + if (gasPricesArray.length > 0) { + return gasPricesArray[0].toString(); + } + return '0uinit'; // Explicit fallbackLikely an incorrect or invalid review comment.
Summary by CodeRabbit
New Features
Chores