fix: improve gas estimation rpc endpoint #1253
Conversation
|
""" WalkthroughA centralized configuration system for Hardhat network settings was introduced via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Module as Backend Module
participant HardhatConfig
participant Web3
Module->>HardhatConfig: get_web3_instance()
HardhatConfig->>HardhatConfig: get_base_url(), get_port()
HardhatConfig->>Web3: Create Web3(HTTPProvider(full_url))
HardhatConfig-->>Module: Return Web3 instance
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
backend/protocol_rpc/endpoint_generator.py (1)
70-91: Potential malformed Hardhat URL – safeguard against double port
HardhatConfig.get_full_url()blindly concatenatesbase_urlandport, so an env value likeHARDHAT_URL=http://hardhat:8545will yieldhttp://hardhat:8545:8545, which therequestscall below will fail on.Quick fix inside
_before_requestscope (untilHardhatConfigis patched):- hardhat_url = HardhatConfig.get_full_url() + raw_url = HardhatConfig.get_full_url() + # strip duplicate port if present (temporary guard) + if raw_url.count(":") > 2: # scheme + host + (>1) port + host_part, _, port_part = raw_url.rpartition(":") + if host_part.endswith(port_part): + raw_url = host_part # port already embedded + hardhat_url = raw_urlLong-term: make
HardhatConfig.get_full_url()detect an existing port and avoid appending a second one.
🧹 Nitpick comments (2)
backend/database_handler/transactions_processor.py (1)
18-19: Consider injecting the Web3 instance instead of constructing it internally
Hard-wiring a new Web3 connection for everyTransactionsProcessorhampers unit-testing (mocking) and forces duplicate connections across services.
Pass a Web3 instance (or theHardhatConfigsingleton) through the constructor and default toHardhatConfig.get_web3_instance()only when none is supplied.Also applies to: 108-109
backend/config/hardhat_config.py (1)
20-25: Consider URL validation to prevent malformed URLs.The
get_full_url()method assumes the base URL doesn't already include a port. If someone setsHARDHAT_URLto"http://localhost:8545", the result would be"http://localhost:8545:8545".Consider adding validation or documentation to clarify the expected format of
HARDHAT_URL.Example improvement:
@staticmethod def get_full_url() -> str: """Get the complete Hardhat URL with port.""" port = HardhatConfig.get_port() url = HardhatConfig.get_base_url() + # Ensure base URL doesn't already include port + if ':' in url.split('://', 1)[1]: + return url return f"{url}:{port}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/config/__init__.py(1 hunks)backend/config/hardhat_config.py(1 hunks)backend/database_handler/transactions_processor.py(2 hunks)backend/protocol_rpc/endpoint_generator.py(3 hunks)backend/rollup/consensus_service.py(2 hunks)tests/hardhat/docker-compose.yml(1 hunks)tests/hardhat/test_hardhat.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
backend/rollup/consensus_service.py (1)
backend/config/hardhat_config.py (2)
HardhatConfig(7-31)get_web3_instance(28-31)
backend/protocol_rpc/endpoint_generator.py (1)
backend/config/hardhat_config.py (2)
HardhatConfig(7-31)get_full_url(21-25)
backend/database_handler/transactions_processor.py (1)
backend/config/hardhat_config.py (2)
HardhatConfig(7-31)get_web3_instance(28-31)
tests/hardhat/test_hardhat.py (1)
backend/config/hardhat_config.py (2)
HardhatConfig(7-31)get_web3_instance(28-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Load Tests / load-test
- GitHub Check: test
- GitHub Check: backend-unit-tests
🔇 Additional comments (6)
backend/config/__init__.py (1)
1-1: Minimal, but fine for package declaration
A single-line docstring is enough to establish the namespace; no further action required for now.tests/hardhat/docker-compose.yml (1)
33-34: Verify compatibility with existing deployments
SplittingHARDHAT_URLandHARDHAT_PORTimproves clarity but will break any environment that still exports a full URL with the port included (e.g.http://hardhat:8545).
Please double-check CI/production env-files and Helm charts before merging.backend/rollup/consensus_service.py (1)
10-20: Checkweb3.is_connected()availability across Web3 versions
web3.is_connected()exists only in Web3 ≥ 6.x; older installations exposeisConnected.
If the project supports both 5.x and 6.x, wrap the call:self.web3_connected = ( self.web3.is_connected() if hasattr(self.web3, "is_connected") else self.web3.isConnected() )(or pin Web3 ≥ 6 in
requirements.txt).tests/hardhat/test_hardhat.py (2)
3-3: Good centralization of Hardhat configuration.The import of
HardhatConfigaligns with the new centralized configuration approach, removing the need for direct environment variable access in test files.
43-43: Excellent refactoring to use centralized Web3 instance creation.The change from manual Web3 instantiation to using
HardhatConfig.get_web3_instance()improves maintainability by centralizing the connection logic. The connection check and error handling remain intact, ensuring the same behavior.backend/config/hardhat_config.py (1)
7-31: Well-designed centralized configuration class.The
HardhatConfigclass provides a clean, centralized approach to managing Hardhat network settings. The static methods, clear docstrings, and sensible defaults make this a maintainable solution.
danieljrc888
force-pushed
the
dxp-365-improve-gas-estimation-rpc-endpoint-2
branch
from
244bf07 to
b338c7b
Compare
danieljrc888
force-pushed
the
dxp-365-improve-gas-estimation-rpc-endpoint-2
branch
from
b338c7b to
70ba9be
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/protocol_rpc/endpoints.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/protocol_rpc/endpoints.py (1)
backend/config/hardhat_config.py (2)
HardhatConfig(7-31)get_web3_instance(28-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Load Tests / load-test
- GitHub Check: test
- GitHub Check: backend-unit-tests
🔇 Additional comments (1)
backend/protocol_rpc/endpoints.py (1)
12-12: LGTM: Import statement is correctly added.The new import for
HardhatConfigis properly added and aligns with the centralized configuration approach mentioned in the AI summary.
| def get_gas_estimate(data: Any) -> str: | ||
| gas_price_in_wei = 30 * 10**6 | ||
| return hex(gas_price_in_wei) | ||
| """ | ||
| Estimate gas for a transaction using Hardhat service. | ||
| Falls back to a default value if the estimation fails. | ||
| """ | ||
| fallback_gas_estimate = 30 * 10**6 | ||
|
|
||
| try: | ||
| web3 = HardhatConfig.get_web3_instance() | ||
| tx_params = {} | ||
| if isinstance(data, dict): | ||
| if "from" in data: | ||
| tx_params["from"] = data["from"] | ||
| if "to" in data: | ||
| tx_params["to"] = data["to"] | ||
| # Handle both 'data' and 'input' fields (some clients use 'input' instead of 'data') | ||
| if "data" in data: | ||
| tx_params["data"] = data["data"] | ||
| elif "input" in data: | ||
| tx_params["data"] = data["input"] | ||
| if "value" in data: | ||
| tx_params["value"] = ( | ||
| int(data["value"], 16) | ||
| if isinstance(data["value"], str) | ||
| else data["value"] | ||
| ) | ||
| if tx_params: | ||
| gas_estimate = web3.eth.estimate_gas(tx_params) | ||
| return hex(gas_estimate) | ||
| else: | ||
| return hex(fallback_gas_estimate) | ||
|
|
||
| except Exception: | ||
| return hex(fallback_gas_estimate) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and parameter validation in gas estimation.
The dynamic gas estimation implementation is a good improvement, but there are several areas that need attention:
-
Overly broad exception handling: The
except Exception:catches all exceptions, which might hide important errors and make debugging difficult. -
Missing parameter validation: The function doesn't validate addresses or other parameters before passing them to Web3.
-
Incomplete value conversion: The hex string conversion assumes proper formatting but doesn't handle edge cases.
-
Missing gas parameter handling: The function doesn't process the "gas" parameter that might be included in the transaction data.
Consider applying these improvements:
def get_gas_estimate(data: Any) -> str:
"""
Estimate gas for a transaction using Hardhat service.
Falls back to a default value if the estimation fails.
"""
fallback_gas_estimate = 30 * 10**6
try:
web3 = HardhatConfig.get_web3_instance()
tx_params = {}
if isinstance(data, dict):
if "from" in data:
tx_params["from"] = data["from"]
if "to" in data:
tx_params["to"] = data["to"]
# Handle both 'data' and 'input' fields (some clients use 'input' instead of 'data')
if "data" in data:
tx_params["data"] = data["data"]
elif "input" in data:
tx_params["data"] = data["input"]
if "value" in data:
- tx_params["value"] = (
- int(data["value"], 16)
- if isinstance(data["value"], str)
- else data["value"]
- )
+ value = data["value"]
+ if isinstance(value, str):
+ # Handle hex strings with or without '0x' prefix
+ tx_params["value"] = int(value, 16) if value.startswith(('0x', '0X')) else int(value, 16)
+ else:
+ tx_params["value"] = value
+ if "gas" in data:
+ gas = data["gas"]
+ if isinstance(gas, str):
+ tx_params["gas"] = int(gas, 16) if gas.startswith(('0x', '0X')) else int(gas, 16)
+ else:
+ tx_params["gas"] = gas
+
if tx_params:
gas_estimate = web3.eth.estimate_gas(tx_params)
return hex(gas_estimate)
else:
return hex(fallback_gas_estimate)
- except Exception:
+ except (ValueError, ConnectionError, TimeoutError) as e:
+ # Log the specific error for debugging
+ print(f"Gas estimation failed: {e}")
+ return hex(fallback_gas_estimate)
+ except Exception as e:
+ # Log unexpected errors
+ print(f"Unexpected error in gas estimation: {e}")
return hex(fallback_gas_estimate)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_gas_estimate(data: Any) -> str: | |
| gas_price_in_wei = 30 * 10**6 | |
| return hex(gas_price_in_wei) | |
| """ | |
| Estimate gas for a transaction using Hardhat service. | |
| Falls back to a default value if the estimation fails. | |
| """ | |
| fallback_gas_estimate = 30 * 10**6 | |
| try: | |
| web3 = HardhatConfig.get_web3_instance() | |
| tx_params = {} | |
| if isinstance(data, dict): | |
| if "from" in data: | |
| tx_params["from"] = data["from"] | |
| if "to" in data: | |
| tx_params["to"] = data["to"] | |
| # Handle both 'data' and 'input' fields (some clients use 'input' instead of 'data') | |
| if "data" in data: | |
| tx_params["data"] = data["data"] | |
| elif "input" in data: | |
| tx_params["data"] = data["input"] | |
| if "value" in data: | |
| tx_params["value"] = ( | |
| int(data["value"], 16) | |
| if isinstance(data["value"], str) | |
| else data["value"] | |
| ) | |
| if tx_params: | |
| gas_estimate = web3.eth.estimate_gas(tx_params) | |
| return hex(gas_estimate) | |
| else: | |
| return hex(fallback_gas_estimate) | |
| except Exception: | |
| return hex(fallback_gas_estimate) | |
| def get_gas_estimate(data: Any) -> str: | |
| """ | |
| Estimate gas for a transaction using Hardhat service. | |
| Falls back to a default value if the estimation fails. | |
| """ | |
| fallback_gas_estimate = 30 * 10**6 | |
| try: | |
| web3 = HardhatConfig.get_web3_instance() | |
| tx_params = {} | |
| if isinstance(data, dict): | |
| if "from" in data: | |
| tx_params["from"] = data["from"] | |
| if "to" in data: | |
| tx_params["to"] = data["to"] | |
| # Handle both 'data' and 'input' fields (some clients use 'input' instead of 'data') | |
| if "data" in data: | |
| tx_params["data"] = data["data"] | |
| elif "input" in data: | |
| tx_params["data"] = data["input"] | |
| if "value" in data: | |
| value = data["value"] | |
| if isinstance(value, str): | |
| # Handle hex strings with or without '0x' prefix | |
| tx_params["value"] = int(value, 16) if value.startswith(('0x', '0X')) else int(value, 16) | |
| else: | |
| tx_params["value"] = value | |
| if "gas" in data: | |
| gas = data["gas"] | |
| if isinstance(gas, str): | |
| tx_params["gas"] = int(gas, 16) if gas.startswith(('0x', '0X')) else int(gas, 16) | |
| else: | |
| tx_params["gas"] = gas | |
| if tx_params: | |
| gas_estimate = web3.eth.estimate_gas(tx_params) | |
| return hex(gas_estimate) | |
| else: | |
| return hex(fallback_gas_estimate) | |
| except (ValueError, ConnectionError, TimeoutError) as e: | |
| # Log the specific error for debugging | |
| print(f"Gas estimation failed: {e}") | |
| return hex(fallback_gas_estimate) | |
| except Exception as e: | |
| # Log unexpected errors | |
| print(f"Unexpected error in gas estimation: {e}") | |
| return hex(fallback_gas_estimate) |
🤖 Prompt for AI Agents
In backend/protocol_rpc/endpoints.py around lines 882 to 915, improve
get_gas_estimate by replacing the broad except Exception with more specific
exception handling to avoid masking errors, validate Ethereum addresses and
other parameters before using them in web3 calls, enhance the conversion of the
"value" field to safely handle edge cases and invalid formats, and add support
to process the "gas" parameter from the input data if present to ensure accurate
gas estimation.
|
Fixes #issue-number-here
What
Why
Testing done
Decisions made
Checks
Reviewing tips
User facing release notes
Summary by CodeRabbit
New Features
Refactor