Problem: no testnet benchmark command #1667
Conversation
temp testnet command changelog
WalkthroughThe changes in this pull request encompass updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1667 +/- ##
===========================================
- Coverage 35.17% 16.88% -18.29%
===========================================
Files 123 72 -51
Lines 11740 6149 -5591
===========================================
- Hits 4129 1038 -3091
+ Misses 7197 4988 -2209
+ Partials 414 123 -291 |
Signed-off-by: yihuang <[email protected]>
yihuang
requested review from
mmsqe and
thomas-nguy
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (3)
testground/benchmark/benchmark/cosmostx.py (1)
81-87: Add class documentation and improve field naming.The class needs documentation to explain its purpose in the Ethereum transaction handling context. Also, consider the following improvements:
- Replace
from_with a standard protobuf naming convention- Add deprecation notice for
deprecated_hashApply this diff:
class MsgEthereumTx(ProtoEntity): + """Represents an Ethereum transaction message in the Cosmos protocol. + + This message type encapsulates Ethereum transactions for processing in the Cosmos chain. + """ MSG_URL = "/ethermint.evm.v1.MsgEthereumTx" data = Field(ProtoAny, 1) + # @deprecated: Use an alternative field (specify which) deprecated_hash = Field("string", 3) - from_ = Field("bytes", 5) + sender = Field("bytes", 5) # 'from' is reserved in protobuf raw = Field("bytes", 6)testground/benchmark/benchmark/utils.py (2)
Line range hint
174-182: Add error handling for Ethereum block retrieval.The function should validate the JSON-RPC response and handle potential errors properly.
Consider adding error handling:
def block_eth(height: int, json_rpc=LOCAL_JSON_RPC): - return requests.post( - json_rpc, - json={ - "jsonrpc": "2.0", - "method": "eth_getBlockByNumber", - "params": [hex(height), False], - "id": 1, - }, - ).json()["result"] + try: + rsp = requests.post( + json_rpc, + json={ + "jsonrpc": "2.0", + "method": "eth_getBlockByNumber", + "params": [hex(height), False], + "id": 1, + }, + timeout=10 + ).json() + if "error" in rsp: + raise ValueError(f"JSON-RPC error: {rsp['error']}") + if "result" not in rsp: + raise ValueError("Invalid JSON-RPC response: missing 'result'") + return rsp["result"] + except (requests.RequestException, ValueError) as e: + raise RuntimeError(f"Failed to fetch Ethereum block {height} from {json_rpc}: {str(e)}")
Line range hint
165-187: Well-structured RPC endpoint configurability.The changes follow a consistent pattern of making RPC endpoints configurable while maintaining backward compatibility. This architectural improvement aligns well with the PR's objective of supporting testnet benchmarking by allowing the functions to work with different networks.
However, consider extracting common RPC error handling logic into a decorator or utility function to maintain DRY principles and ensure consistent error handling across all RPC calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- CHANGELOG.md (1 hunks)
- testground/benchmark/benchmark/cosmostx.py (1 hunks)
- testground/benchmark/benchmark/stats.py (2 hunks)
- testground/benchmark/benchmark/testnet.py (1 hunks)
- testground/benchmark/benchmark/transaction.py (7 hunks)
- testground/benchmark/benchmark/utils.py (2 hunks)
- testground/benchmark/flake.nix (1 hunks)
- testground/benchmark/pyproject.toml (1 hunks)
🔇 Additional comments (12)
testground/benchmark/flake.nix (2)
36-39: LGTM! Clean addition of the testnet app.The new
testnetapp entry follows the established pattern and maintains consistency with existing app declarations.
36-39: Verify the testnet binary in benchmark-testcase package.Let's ensure the referenced binary
testnetis properly installed in the package.✅ Verification successful
Let me check the content of the testnet.py file and look for any binary installation or build configurations.
Let me check the build configuration in pyproject.toml to verify how the testnet binary is defined.
Let me check the overlay.nix file to see how the benchmark-testcase package is built.
The testnet binary is properly configured and will be installed correctly
The verification shows that:
- The
testnetscript is properly defined inpyproject.tomlunder[tool.poetry.scripts]section, pointing tobenchmark.testnet:cli- The implementation exists in
testground/benchmark/benchmark/testnet.pywith a proper CLI entrypoint- The nix package is correctly configured to build using poetry2nix and expose the binary through the
apps.testnetattribute inflake.nix🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the testnet binary installation in the package # Test 1: Check if the binary is declared in setup files echo "Checking setup files for testnet binary declaration..." rg -l "testnet" --type python | grep -E "setup\.py|pyproject\.toml" # Test 2: Look for the testnet.py source file echo "Looking for testnet implementation..." fd testnet.pyLength of output: 360
Script:
#!/bin/bash # Check the content of testnet.py cat testground/benchmark/benchmark/testnet.py # Look for any build or installation configurations fd -e nix -e toml -e cabal . | xargs rg -l "testnet"Length of output: 4327
Script:
#!/bin/bash # Check pyproject.toml for binary configuration cat testground/benchmark/pyproject.toml # Check the nix package definition cat testground/benchmark/flake.nixLength of output: 2272
Script:
#!/bin/bash # Check the package overlay definition cat testground/benchmark/overlay.nixLength of output: 1560
testground/benchmark/benchmark/stats.py (3)
3-3: LGTM: Import changes are appropriate.The new imports support the configurable RPC endpoints feature.
22-26: LGTM: RPC endpoint configuration added correctly.The function properly handles the new
rpcparameter. Let's verify its usage across the codebase.#!/bin/bash # Description: Check if there are any direct calls to get_block_info_cosmos without the rpc parameter # This could identify places that need to be updated ast-grep --pattern 'get_block_info_cosmos($height)' || echo "No calls found with single parameter"
29-33: LGTM: JSON-RPC endpoint configuration added correctly.The function properly handles the new
json_rpcparameter. Let's verify its usage across the codebase.✅ Verification successful
LGTM: All calls to
get_block_info_ethproperly include thejson_rpcparameterThe search results show that the function is only called once in the codebase at
testground/benchmark/benchmark/stats.py, and this call correctly includes thejson_rpcparameter. No updates are needed in other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any direct calls to get_block_info_eth without the json_rpc parameter # This could identify places that need to be updated ast-grep --pattern 'get_block_info_eth($height)' || echo "No calls found with single parameter"Length of output: 50
Script:
#!/bin/bash # Let's try a more comprehensive search for any calls to get_block_info_eth rg "get_block_info_eth" -A 2Length of output: 559
Script:
#!/bin/bash # Also search for function definition to understand its usage pattern ast-grep --pattern 'def get_block_info_eth($$$)'Length of output: 50
Script:
#!/bin/bash # Search for any test files that might use this function fd "test.*\.py" --exec grep -l "get_block_info_eth" {}Length of output: 56
testground/benchmark/benchmark/cosmostx.py (1)
89-100: 🛠️ Refactor suggestionAdd documentation and field validation for Ethereum transaction parameters.
The class needs documentation and field validation to ensure transaction integrity:
- Add class documentation explaining the legacy transaction format
- Add validation for hex string fields
- Add size validation for signature components
Apply this diff:
class LegacyTx(ProtoEntity): + """Represents a legacy Ethereum transaction (EIP-155). + + Contains all fields necessary for a pre-EIP-2718 Ethereum transaction: + - nonce: Account transaction sequence + - gas_price: Price per gas unit in wei + - gas: Gas limit + - to: Recipient address (hex string) + - value: Transaction value in wei + - data: Transaction input data + - v, r, s: Transaction signature components + """ MSG_URL = "/ethermint.evm.v1.LegacyTx" nonce = Field("uint64", 1) gas_price = Field("string", 2) gas = Field("uint64", 3) to = Field("string", 4) value = Field("string", 5) data = Field("bytes", 6) v = Field("bytes", 7) r = Field("bytes", 8) s = Field("bytes", 9) + + def validate(self): + """Validate transaction fields.""" + if self.to and not self.to.startswith('0x'): + raise ValueError("'to' address must be hex string starting with '0x'") + if not self.value.isnumeric(): + raise ValueError("'value' must be a numeric string") + for sig in (self.r, self.s): + if len(sig) != 32: + raise ValueError("r, s must be 32 bytes")Let's verify if there are any existing validation implementations:
testground/benchmark/benchmark/utils.py (1)
186-187: LGTM!The function correctly propagates the RPC endpoint to the
blockfunction and maintains a clean separation of concerns.CHANGELOG.md (1)
12-12: LGTM!The changelog entry is well-formatted, concise, and correctly documents the addition of the testnet benchmark command.
testground/benchmark/benchmark/transaction.py (4)
44-52: Enhancement ofsimple_transfer_txfunction improves flexibilityThe addition of the
optionsparameter and the dynamic retrieval ofgas_priceandchain_idallow for more flexible transaction configurations.
55-66: Enhancement oferc20_transfer_txfunction improves flexibilityThe addition of the
optionsparameter and the dynamic retrieval ofgas_priceandchain_idallow for more flexible transaction configurations.
75-116: Introduction ofbuild_evm_msg_1_3andbuild_evm_msg_1_4adds version supportThe new functions for building Ethereum messages for different versions enhance compatibility with various protocol versions.
243-246: Addition ofrpcandsyncparameters toasync_sendtxincreases flexibilityAllowing the specification of the RPC endpoint and choosing between synchronous and asynchronous broadcasting enhance the function's usability.
Signed-off-by: yihuang <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
0f94930
temp
testnet command
changelog
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation