Problem: unable to query historical txs whose module is removed #1713
Conversation
WalkthroughThe changes in this pull request introduce several updates across multiple files, primarily focusing on the registration of legacy codecs and interfaces to support querying historical transactions from removed modules. The Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar." 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 #1713 +/- ##
===========================================
+ Coverage 16.87% 35.56% +18.68%
===========================================
Files 72 126 +54
Lines 6163 11915 +5752
===========================================
+ Hits 1040 4237 +3197
- Misses 5000 7254 +2254
- Partials 123 424 +301
|
mmsqe
requested review from
yihuang and
leejw51crypto
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: 5
🧹 Outside diff range and nitpick comments (5)
integration_tests/utils.py (1)
828-868: Enhance logging and exception handling indo_upgradefunctionThe
do_upgradefunction manages critical operations without sufficient logging or error handling.Add detailed logging messages and exception handling to improve traceability and reliability.
def do_upgrade(c, plan_name, target, mode=None): print(f"Initiating upgrade '{plan_name}' at height: {target}") cli = c.cosmos_cli() try: if plan_name == "v1.4.0-rc5-testnet": rsp = cli.software_upgrade( "community", { "name": plan_name, "title": "upgrade test", "note": "ditto", "upgrade-height": target, "summary": "summary", "deposit": "10000basetcro", }, ) events = rsp["events"] else: rsp = cli.gov_propose_legacy( "community", "software-upgrade", { "name": plan_name, "title": "upgrade test", "description": "ditto", "upgrade-height": target, "deposit": "10000basetcro", }, mode=mode, ) events = rsp["logs"][0]["events"] assert rsp["code"] == 0, rsp["raw_log"] approve_proposal(c, events) # Update cli chain binary c.chain_binary = ( Path(c.chain_binary).parent.parent.parent / f"{plan_name}/bin/cronosd" ) # Wait for the upgrade to complete wait_for_block(c.cosmos_cli(), target + 2, timeout=480) wait_for_port(ports.rpc_port(c.base_port(0))) print(f"Upgrade '{plan_name}' completed successfully.") except Exception as e: print(f"Error during upgrade '{plan_name}': {e}") raise return c.cosmos_cli()integration_tests/configs/upgrade-test-package-recent.nix (1)
13-14: Consider using tagged releases instead of commit hashes.The commit hashes are hardcoded for specific versions. Consider using tagged releases for better maintainability and clarity.
- releasedGenesis = - (fetchFlake "crypto-org-chain/cronos" "e1d819c862b30f0ce978baf2addb12516568639e").default; + releasedGenesis = + (fetchFlake "crypto-org-chain/cronos" "v1.3.0").default; - released1_4 = - (fetchFlake "crypto-org-chain/cronos" "ce797fa995000530ee53cd1fbeb3c67180648002").default; + released1_4 = + (fetchFlake "crypto-org-chain/cronos" "v1.4.0").default;Also applies to: 16-17
integration_tests/test_upgrade_with_ibc.py (3)
81-82: Enhance transaction verification after upgrade.The test verifies transaction data equality but could be more specific about what aspects of the transaction need to be preserved.
- ica_bf = get_tx(base_port, ica_txhash) - cli = do_upgrade(c, "v1.4", cli.block_height() + 15) + ica_bf = get_tx(base_port, ica_txhash) + # Store specific fields we want to verify + important_fields = { + 'tx_response': { + 'code': ica_bf['tx_response']['code'], + 'events': [e for e in ica_bf['tx_response']['events'] if e['type'] == 'register_account'], + } + } + cli = do_upgrade(c, "v1.4", cli.block_height() + 15) - ica_af = get_tx(base_port, ica_txhash) - assert ica_bf == ica_af, ica_af + ica_af = get_tx(base_port, ica_txhash) + # Verify specific fields instead of entire response + assert ica_af['tx_response']['code'] == important_fields['tx_response']['code'], "Transaction status changed after upgrade" + assert any(e['type'] == 'register_account' for e in ica_af['tx_response']['events']), "Register account event missing after upgrade"Also applies to: 88-89
66-71: Add retry mechanism for ICA registration.The ICA registration should handle temporary network issues.
+ from tenacity import retry, stop_after_attempt, wait_exponential + + @retry(stop=stop_after_attempt(3), wait=wait_exponential(multiplier=1, min=4, max=10)) + def register_ica_account(cli, connid, signer, version): + return cli.icaauth_register_account( + connid, + from_=signer, + gas="400000", + version=version, + ) - rsp = cli.icaauth_register_account( - connid, - from_=signer, - gas="400000", - version=v, - ) + rsp = register_ica_account(cli, connid, signer, v)
84-85: Add descriptive assertion message.The assertion should explain why querying icaauth params is expected to fail.
- with pytest.raises(AssertionError): - cli.query_params("icaauth") + with pytest.raises(AssertionError, match="icaauth module should be removed after upgrade"): + cli.query_params("icaauth")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
integration_tests/poetry.lockis excluded by!**/*.lockx/icaauth/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
CHANGELOG.md(1 hunks)app/app.go(2 hunks)integration_tests/configs/cosmovisor_with_ibc.jsonnet(1 hunks)integration_tests/configs/upgrade-test-package-recent.nix(1 hunks)integration_tests/cosmoscli.py(1 hunks)integration_tests/ibc_utils.py(2 hunks)integration_tests/network.py(1 hunks)integration_tests/pyproject.toml(1 hunks)integration_tests/test_upgrade.py(5 hunks)integration_tests/test_upgrade_with_ibc.py(1 hunks)integration_tests/utils.py(2 hunks)nix/sources.json(1 hunks)nix/testenv.nix(1 hunks)x/icaauth/types/codec.go(1 hunks)
🔇 Additional comments (22)
integration_tests/test_upgrade.py (6)
19-22: Imports updated to include essential functions
The added imports (do_upgrade, get_consensus_params, get_send_enable, post_init) are necessary for the new functionalities introduced in the test upgrade process. This enhances modularity by reusing code from utils.py.
132-132: Correct usage of do_upgrade with updated signature
The do_upgrade function is now called with the additional c parameter (cli = do_upgrade(c, "v1.1.0", target_height0, "block")), aligning with the updated function signature. This ensures the upgrade process utilizes the current context of the Cronos instance.
155-155: Consistent application of do_upgrade function
The repeated use of do_upgrade (cli = do_upgrade(c, "v1.2", target_height1)) maintains consistency in the upgrade testing process. This standardizes how upgrades are executed throughout the tests.
211-215: Potential issue with transaction comparison after upgrade
The assertion assert txs == get_txs(base_port, height) compares transactions before and after the upgrade. Upgrades may alter transaction results or introduce state changes that could cause this assertion to fail.
Ensure that the upgrade does not affect the transaction outputs or adjust the assertion to account for acceptable changes post-upgrade.
215-215: Initiating upgrade to v1.4
The upgrade to v1.4 (cli = do_upgrade(c, "v1.4", cli.block_height() + 15)) is correctly initiated. Verify that any changes introduced in v1.4 are compatible with existing tests and functionalities.
226-226: Upgrade to v1.4.0-rc5-testnet requires thorough testing
Upgrading to a release candidate version (cli = do_upgrade(c, "v1.4.0-rc5-testnet", cli.block_height() + 15)) may introduce new features or changes that need validation.
Consider adding specific tests to cover new features in v1.4.0-rc5-testnet to ensure comprehensive validation.
integration_tests/utils.py (1)
27-27: Importing ports module for port management
The added import (from pystarport import ledger, ports) brings in the ports module, which is necessary for the new utility functions that involve port operations.
x/icaauth/types/codec.go (2)
10-13: Registering message types with the Legacy Amino codec
The RegisterCodec function correctly registers MsgRegisterAccount and MsgSubmitTx messages, enabling serialization and deserialization of these message types using the Legacy Amino codec.
15-23: Registering interfaces and message service descriptors
The RegisterInterfaces function properly registers the message implementations and the message service descriptor, ensuring seamless integration with the Cosmos SDK's interface registry.
integration_tests/configs/cosmovisor_with_ibc.jsonnet (2)
9-15: Inconsistent configuration across validators
The configuration modifies the first validator's settings but not the others. This could lead to inconsistent behavior in the test environment.
Confirm whether the custom store settings are intentionally applied only to the first validator, or update the configuration to apply them to all validators if uniformity is desired.
16-30: Consensus parameters and governance adjustments are appropriate
The additions to consensus_params and modifications to gov parameters enhance the network configuration to meet specific testing requirements.
integration_tests/pyproject.toml (1)
31-31: Adding rpds-py dependency
The inclusion of rpds-py = "0.17.1" adds rpds-py to the project's dependencies.
Ensure that this package is necessary and compatible with existing dependencies. Check for any potential conflicts or redundancy.
nix/testenv.nix (1)
22-25: LGTM! Verify dependency requirements.
The addition of the docker build system with hatchling and hatch-vcs follows the existing pattern correctly.
Let's verify if these dependencies are actually required:
integration_tests/network.py (1)
189-196: LGTM: Improved chain binary selection logic.
The changes introduce a more robust way to handle the chain binary selection with proper fallback mechanisms.
Let's verify the chain binary selection logic:
✅ Verification successful
Chain binary selection logic is correctly implemented and used consistently
The verification confirms that the chain binary selection logic is properly implemented:
- The default value "cronosd" is used consistently across the codebase as shown in
network.pyand other test files - The comma-separated chain binary handling is used correctly in upgrade scenarios (e.g., in
test_upgrade_with_ibc.pywithchain-maind,{binary}) - The fallback mechanism to "cronosd" aligns with all binary usages in the integration tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the chain binary selection logic by checking for any direct usage of cronosd.
# Check for direct usage of cronosd binary
rsp=$(rg -n "cronosd" --type py)
echo "Direct usage of cronosd binary in Python files:"
echo "$rsp"
# Check for chain binary configuration in test files
rsp=$(rg -n "chain_binary" --type py)
echo "Chain binary configuration in Python files:"
echo "$rsp"
Length of output: 10827
nix/sources.json (1)
117-120: Verify the poetry2nix version update.
The poetry2nix package has been updated to a newer version. Please ensure this update doesn't introduce any breaking changes.
Let's verify the version update:
✅ Verification successful
Based on the commit history between the versions, I can generate the final response:
Poetry2nix update appears safe to proceed
The update primarily includes:
- Build system fixes and overrides for various Python packages
- Dependency updates and build system migrations
- No breaking changes in core functionality
The changes are mostly maintenance-related, adding support for more packages and fixing build issues. The update improves compatibility with newer package versions and build systems.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any breaking changes in the poetry2nix update
# Get the changelog between the old and new versions
gh api repos/nix-community/poetry2nix/compare/4eb2ac54029af42a001c9901194e9ce19cbd8a40...f554d27c1544d9c56e5f1f8e2b8aff399803674e \
--jq '.commits[].commit.message'
Length of output: 15818
integration_tests/ibc_utils.py (2)
152-153: LGTM: Added new parameters for network preparation.
The addition of post_init and chain_binary parameters enhances the flexibility of network setup.
167-168: LGTM: Proper parameter forwarding.
The new parameters are correctly forwarded to setup_custom_cronos.
CHANGELOG.md (1)
5-8: LGTM! Clear and well-formatted changelog entry.
The entry follows the standard format and clearly describes the purpose of registering the legacy codec for querying historical transactions from the deleted icaauth module.
app/app.go (2)
173-173: LGTM! Import statement for icaauthtypes.
The import is required for registering the legacy codec to support historical transaction queries.
854-855: LGTM! Registration of icaauth codec and interfaces.
The registration of codec and interfaces is necessary for proper serialization and type handling of historical icaauth transactions.
integration_tests/cosmoscli.py (2)
1440-1459: LGTM! Well-structured icaauth_register_account implementation.
The method follows the established patterns for transaction commands in the codebase:
- Uses consistent default kwargs
- Handles transaction response and event querying properly
- Maintains error handling patterns
1461-1473: LGTM! Well-structured icaauth_query_account implementation.
The method follows the established patterns for query commands in the codebase:
- Uses consistent default kwargs
- Returns properly formatted JSON response
- Maintains consistent parameter handling
yihuang
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
app/legacy/icaauth/types/codec.go (1)
1-23: Well-structured implementation for legacy transaction supportThe implementation effectively addresses the PR objective of enabling historical transaction queries for removed modules. The codec and interface registrations follow Cosmos SDK patterns and ensure backward compatibility.
Consider documenting these legacy codec registrations in the module's README or documentation to help future maintainers understand the purpose and importance of these registrations for historical transaction support.
app/legacy.go (2)
10-13: Consider adding documentation for exported functionThe function implementation is correct, but as an exported function, it should include documentation explaining its purpose and usage.
Add documentation like:
+// RegisterLegacyCodec registers legacy transaction types into the amino codec to support +// backwards compatibility with historical transactions from removed modules (icaauth, authz). func RegisterLegacyCodec(cdc *codec.LegacyAmino) {
15-18: Consider adding documentation for exported functionSimilar to RegisterLegacyCodec, this exported function should be documented.
Add documentation like:
+// RegisterLegacyInterfaces registers interface types for historical transaction messages +// from removed modules (icaauth, authz) to support querying old transactions. func RegisterLegacyInterfaces(registry cdctypes.InterfaceRegistry) {app/legacy_test.go (2)
12-30: Enhance test readability and coverageWhile the test correctly verifies legacy transaction decoding, there are several improvements that could be made:
- The base64 encoded transaction string should be documented with its contents
- The test should verify the decoded transaction's content
- Consider extracting the base64 string to a test constant
Consider refactoring like this:
+const legacyICAAuthTx = `Cl8KXQoeL2ljYWF1dGgudjEuTXNnUmVnaXN0ZXJBY2NvdW50EjsKK3Rj...` + func TestDecodeLegacyTx(t *testing.T) { - // msg url: /icaauth.v1.MsgRegisterAccount - tx := "Cl8KXQoeL2ljYWF1dGgudjEuTXNnUmVnaXN0ZXJBY2NvdW50Ej..." + // Test decoding a legacy ICAAuth MsgRegisterAccount transaction + // Expected contents: + // - From: tcrc1rwl5nxlrxewyxejd0898uqaj4796v42ma50yp4 + // - Connection ID: connection-0 + tx := legacyICAAuthTx bz, err := base64.StdEncoding.DecodeString(tx) require.NoError(t, err) encodingConfig := evmenc.MakeConfig() // First attempt should fail due to missing codec _, err = encodingConfig.TxConfig.TxDecoder()(bz) require.Error(t, err, "") require.Contains(t, err.Error(), "unable to resolve type URL /icaauth.v1.MsgRegisterAccount") RegisterLegacyCodec(encodingConfig.Amino) RegisterLegacyInterfaces(encodingConfig.InterfaceRegistry) - _, err = encodingConfig.TxConfig.TxDecoder()(bz) + decodedTx, err := encodingConfig.TxConfig.TxDecoder()(bz) require.NoError(t, err) + + // Verify decoded transaction contents + msgs := decodedTx.GetMsgs() + require.Len(t, msgs, 1) + msg := msgs[0].(*icaauthtypes.MsgRegisterAccount) + require.Equal(t, "tcrc1rwl5nxlrxewyxejd0898uqaj4796v42ma50yp4", msg.From) + require.Equal(t, "connection-0", msg.ConnectionID) }
32-52: Similar improvements needed for AuthzLegacyTx testThe same improvements suggested for TestDecodeLegacyTx apply here.
Additionally, consider extracting the common test setup into a helper function:
+func setupEncodingConfig(t *testing.T) encoding.Config { + encodingConfig := evmenc.MakeConfig() + RegisterLegacyCodec(encodingConfig.Amino) + RegisterLegacyInterfaces(encodingConfig.InterfaceRegistry) + return encodingConfig +} func TestDecodeAuthzLegacyTx(t *testing.T) { + // Test decoding a legacy Authz MsgGrant transaction with SendAuthorization + // Expected contents: + // - Granter: crc1x7x9pkfxf33l87ftspk5aetwnkr0lvlv3346cd + // - Grantee: crc16z0herz999846wr659lr84c8c556da55dc34hh + // - Authorization: SendAuthorization with spend limit of 200basetcro ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/legacy/icaauth/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)app/app.go(1 hunks)app/legacy.go(1 hunks)app/legacy/icaauth/types/codec.go(1 hunks)app/legacy_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- app/app.go
🔇 Additional comments (5)
app/legacy/icaauth/types/codec.go (3)
1-8: LGTM: Package structure and imports follow Cosmos SDK conventions
The package name and imports are appropriate for codec registration functionality.
10-13: Verify completeness of message type registration
The function correctly registers MsgRegisterAccount and MsgSubmitTx using the standard Cosmos SDK pattern. However, we should verify that these are all the message types that need registration for historical transaction querying.
✅ Verification successful
Let me try a different search strategy to find message types in the codebase.
Message type registration is complete and correct
The search results confirm that MsgRegisterAccount and MsgSubmitTx are the only message types defined in the icaauth module, and both are properly registered in the codec. The registration is complete as it covers all existing message types that require amino codec registration for historical transaction querying.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all message types in the icaauth module that might need registration
# Look for message definitions in both current and git history
# Search for message type definitions in current codebase
echo "Current message types in icaauth:"
ast-grep --pattern 'type Msg$_ struct {'
# Search in git history for previously existing message types
echo -e "\nHistorical message types in icaauth:"
git log -p | rg "type Msg.*struct.*{" | sort -u
Length of output: 208
Script:
#!/bin/bash
# Search for message type definitions using ripgrep with more context
echo "Searching for message types in icaauth module:"
rg -A 2 "type Msg.*struct" app/legacy/icaauth/
# Search for any protobuf message definitions
echo -e "\nSearching for protobuf message definitions:"
rg "message Msg" app/legacy/icaauth/
# Look for any other codec registrations in the module
echo -e "\nSearching for other codec registrations:"
rg "RegisterConcrete" app/legacy/icaauth/
Length of output: 1560
15-23: Consider combining RegisterImplementations calls
While the current implementation is functionally correct, we can optimize it by combining the RegisterImplementations calls:
func RegisterInterfaces(registry cdctypes.InterfaceRegistry) {
- registry.RegisterImplementations((*sdk.Msg)(nil),
- &MsgRegisterAccount{},
- )
- registry.RegisterImplementations((*sdk.Msg)(nil),
- &MsgSubmitTx{},
- )
+ registry.RegisterImplementations((*sdk.Msg)(nil),
+ &MsgRegisterAccount{},
+ &MsgSubmitTx{},
+ )
msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc)
}app/legacy.go (1)
3-9: LGTM! Imports are appropriate and well-organized.
The imports include necessary packages for codec registration and types from both Cosmos SDK and Cronos.
app/legacy_test.go (1)
14-14: Verify test data sensitivity and coverage
Please ensure:
- The base64 encoded transactions don't contain sensitive information (e.g., private keys)
- Test cases cover all relevant scenarios for removed modules
Let's decode and inspect the test transactions:
Also applies to: 34-34
✅ Verification successful
Let me try a different approach to decode and analyze the transactions:
Based on the analysis of the test files and transactions, I can now provide a final response:
Test data verified as safe and comprehensive
-
The base64 encoded transactions only contain public data:
- Public keys (ethsecp256k1)
- Account addresses (tcrc1...)
- Message types and parameters
- No private keys or sensitive credentials present
-
Test coverage is complete for removed modules:
- Tests
TestDecodeLegacyTxfor ICAAuth module - Tests
TestDecodeAuthzLegacyTxfor Authz module - Both tests verify proper handling of legacy transaction decoding
- Tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Decode and inspect base64 encoded test transactions
echo "Decoding ICAAuth transaction..."
echo "Cl8KXQoeL2ljYWF1dGgudjEuTXNnUmVnaXN0ZXJBY2NvdW50EjsKK3RjcmMxcndsNW54bHJ4ZXd5eGVqZDA4OTh1cWFqNDc5NnY0Mm1hNTB5cDQSDGNvbm5lY3Rpb24tMBKAAQpXCk8KKC9ldGhlcm1pbnQuY3J5cHRvLnYxLmV0aHNlY3AyNTZrMS5QdWJLZXkSIwohA1Q2Pphmzen/aIkuFd/3k+8YQATsARLhmWV9RxF+FGS/EgQKAggBEiUKHwoIYmFzZXRjcm8SEzQ4NDgwMDAwMDAwMDAwMDAwMDAQgLUYGkHOZPyLl81RsTbNHTv4o4XjWtYwO1fm4NzYyuju4boHpmALIytbPm+saXwhxtUG6hPT+sAsu9Bk224A9xd/8isZAQ==" | base64 -d | xxd -p -c0
echo -e "\nDecoding Authz transaction..."
echo "Cr0BCroBCh4vY29zbW9zLmF1dGh6LnYxYmV0YTEuTXNnR3JhbnQSlwEKKmNyYzF4N3g5cGtmeGYzM2w4N2Z0c3BrNWFldHdua3IwbHZsdjMzNDZjZBIqY3JjMTZ6MGhlcno5OTg5NDZ3cjY1OWxyODRjOGM1NTZkYTU1ZGMzNGhoGj0KOwomL2Nvc21vcy5iYW5rLnYxYmV0YTEuU2VuZEF1dGhvcml6YXRpb24SEQoPCghiYXNldGNybxIDMjAwEl8KVwpPCigvZXRoZXJtaW50LmNyeXB0by52MS5ldGhzZWNwMjU2azEuUHViS2V5EiMKIQNg/r8Tea2PYFq2XE07fpnN97ASePg32cuO4HmUkEGpFBIECgIIARIEEMCaDBpBekTx2LtLIFDODLVr1OqMUR9UYjZBv0KAr8eCacs7jTw/szr3jCvtvHGNraifkLfBEDatH3Rp/wqaNvxXgGjH4gA=" | base64 -d | xxd -p -c0
# Check for other test files that might contain similar transactions
echo -e "\nChecking for other test files with base64 encoded transactions..."
rg -l 'base64\.StdEncoding\.DecodeString' --type go
Length of output: 1687
Script:
#!/bin/bash
# Search for test files containing transactions and analyze their content
echo "Searching for test files and their content..."
rg -A 10 "MsgRegisterAccount|MsgGrant" --type go
# Look for any potential sensitive patterns in test files
echo -e "\nChecking for potential sensitive patterns..."
rg -i "private.*key|secret|password|credential" --type go app/legacy_test.go
# Check test coverage
echo -e "\nAnalyzing test scenarios..."
rg -B 2 -A 2 "func.*Test" app/legacy_test.go
Length of output: 26119
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/legacy_test.go (2)
13-16: Add documentation for test cases.The test data map contains base64-encoded transactions, but it's not immediately clear what these transactions represent or what specific scenarios they're testing. Consider adding comments to document:
- The purpose of each test case
- The expected decoded content
- Why these specific message types were chosen
Example improvement:
txs := map[string]string{ + // Test case 1: Legacy ICA registration transaction + // Expected content: MsgRegisterAccount with connection-0 "/icaauth.v1.MsgRegisterAccount": "Cl8KXQoeL2ljYWF1...", + // Test case 2: Legacy authz grant transaction + // Expected content: MsgGrant with SendAuthorization "/cosmos.authz.v1beta1.MsgGrant": "Cr0BCroBCh4vY29...", }
12-36: Add test coverage for edge cases.Consider adding test cases for the following scenarios:
- Invalid base64 encoded data
- Malformed transaction data
- Unsupported message types
- Empty transaction data
Example additional test cases:
"invalid_base64": { typeUrl: "/icaauth.v1.MsgRegisterAccount", tx: "invalid-base64", expectError: true, }, "malformed_tx": { typeUrl: "/icaauth.v1.MsgRegisterAccount", tx: base64.StdEncoding.EncodeToString([]byte("malformed")), expectError: true, }, "unsupported_type": { typeUrl: "/unknown.v1.Message", tx: validEncodedTx, expectError: true, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/legacy_test.go(1 hunks)
🔇 Additional comments (1)
app/legacy_test.go (1)
1-10: LGTM! Package and imports are well-structured.
The necessary imports are included for testing, encoding/decoding, and assertions.
…to-org-chain#1713) * Problem: unable to query historical txs from deleted icaauth module * test * poetry * deps * add unit tets * cleanup * rename * revert integration test * revert * add authz * changelog * test authz * cleanup * register in query * only patch encoding for tx service * Revert "only patch encoding for tx service" This reverts commit 4b1c141. --------- Co-authored-by: HuangYi <[email protected]>
* Problem: rocksdb 9.8.4 is not used (#1712) for more info, facebook/rocksdb@v9.7.4...v9.8.4 * Problem: query blocks before enable feemarket module get nil pointer error (#1714) * Problem: unable to query historical txs whose module is removed (#1713) * Problem: unable to query historical txs from deleted icaauth module * test * poetry * deps * add unit tets * cleanup * rename * revert integration test * revert * add authz * changelog * test authz * cleanup * register in query * only patch encoding for tx service * Revert "only patch encoding for tx service" This reverts commit 4b1c141. --------- Co-authored-by: HuangYi <[email protected]> --------- Co-authored-by: HuangYi <[email protected]>
👮🏻👮🏻👮🏻 !!!! 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
Bug Fixes
Improvements
rocksdbdependency to versionv9.8.4.Tests