fix: contstuction payloads #190
Conversation
WalkthroughThis pull request introduces modifications to the Rosetta implementation, focusing on enhancing address conversion and codec handling. The changes primarily involve updating the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/systemtests/construction_test.go (1)
108-111: Enhance payload validation in test.The test should validate more aspects of the constructed payload beyond just checking for non-empty unsigned transaction.
Consider adding these validations:
- Verify the transaction type matches MsgSend
- Validate the amount and denomination in the response
- Check the sequence number matches the input
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
client_online.go(1 hunks)converter.go(6 hunks)converter_test.go(1 hunks)tests/systemtests/accounts_test.go(1 hunks)tests/systemtests/block_test.go(1 hunks)tests/systemtests/construction_test.go(3 hunks)tests/systemtests/main_test.go(1 hunks)tests/systemtests/mempool_test.go(1 hunks)tests/systemtests/network_test.go(1 hunks)tests/systemtests/rest_client.go(2 hunks)tests/systemtests/rosetta.go(2 hunks)tests/systemtests/test_runner.go(2 hunks)utils.go(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- tests/systemtests/main_test.go
- utils.go
- tests/systemtests/accounts_test.go
- tests/systemtests/network_test.go
- tests/systemtests/block_test.go
- tests/systemtests/mempool_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: rosetta-cli-test
🔇 Additional comments (10)
converter.go (3)
19-19: LGTM: Import of address codecThe addition of the address codec import aligns with the PR's objective to improve address handling.
111-111: LGTM: New address codec fieldThe addition of the
acfield to store the address codec instance is well-placed and properly initialized.Also applies to: 136-136
683-683: Fix implemented for signer retrieval and address conversionThe changes correctly implement the fix by:
- Using
GetSigners()instead ofGetSignaturesV2()- Properly converting signer bytes to Bech32 address format
- Improving error messages for debugging
Also applies to: 713-715, 722-726, 729-729, 743-743
client_online.go (1)
100-100: LGTM: Consistent usage of address codecThe update to include
address.NewBech32Codec(cfg.Bech32Prefix)in the NewConverter call is consistent with the changes in converter.go.converter_test.go (1)
47-47: LGTM: Test updated for new converter signatureThe test setup is properly updated to include the Bech32 codec parameter, maintaining test coverage for the modified functionality.
tests/systemtests/test_runner.go (1)
1-1: Package name change follows consistent naming pattern.The package name has been updated to
rossetaSystemTeststo better reflect its purpose and maintain consistency across the test suite.tests/systemtests/rest_client.go (2)
160-172: LGTM! Well-structured request body construction.The
constructionPayloadsmethod properly formats the request body and includes appropriate error handling.
174-181: LGTM! Clean and focused type definition.The
operationtype and itsString()method are well-designed and serve their purpose effectively.tests/systemtests/rosetta.go (1)
48-48: Function signature change aligns with removal of offline mode.The updated signature of
newRosettaRunnercorrectly reflects the removal of the offline functionality, maintaining consistency with the changes in other files.tests/systemtests/construction_test.go (1)
34-35: LGTM! Improved address generation.The updated address generation using
NewBech32Codecis more robust and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/systemtests/construction_test.go (1)
108-108: Use structured data instead of hardcoded JSON string.The hardcoded JSON string for signer_data is error-prone and difficult to maintain.
- res, err := rosettaRest.constructionPayloads(`"signer_data":[{"account_number":1, "sequence": 0}]`, hexPk, op) + signerData := map[string]interface{}{ + "signer_data": []map[string]interface{}{ + { + "account_number": 1, + "sequence": 0, + }, + }, + } + res, err := rosettaRest.constructionPayloads(signerData, hexPk, op)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)tests/systemtests/construction_test.go(3 hunks)tests/systemtests/rest_client.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/systemtests/rest_client.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: golangci-lint
- GitHub Check: rosetta-cli-test
🔇 Additional comments (3)
CHANGELOG.md (1)
48-51: LGTM! The changelog entry is well-structured and informative.The new entry follows the changelog guidelines, properly documents the bug fix, and provides clear context about the changes made to handle transaction signers correctly.
tests/systemtests/construction_test.go (2)
34-35: LGTM! Good use of the new address codec.The changes correctly implement the new Bech32Codec for address conversion, improving the address handling in the test.
108-111: Enhance test coverage for the signer fix.Based on the PR objectives to fix "unsigned transaction signatures instead of signers", the test should explicitly verify that the correct signers are returned instead of signatures.
Please add assertions to validate:
- The presence of signers in the response
- The absence of signatures
- The correct format of the signer data
assert.NotEmpty(t, gjson.GetBytes(res, "unsigned_transaction")) assert.Equal(t, gjson.GetBytes(res, "payloads.0.address").String(), addr) + // Verify the fix for signatures vs signers + assert.True(t, gjson.GetBytes(res, "payloads.0.signer_type").Exists(), "signer_type should be present") + assert.False(t, gjson.GetBytes(res, "payloads.0.signature_type").Exists(), "signature_type should not be present")
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/systemtests/rosetta.go (1)
Plugin configuration is collected but not utilized
The codebase shows an inconsistency where the Plugin field and CLI flag (
-rosetta-plugin) are still present and actively used, but the--pluginargument is not forwarded to the Rosetta binary in thestartmethod. This suggests either:
- A missing plugin configuration in the startup arguments
- Or an incomplete cleanup of deprecated plugin functionality
Please verify:
- If plugins are still supported, restore the
--pluginargument in the startup configuration- If plugins are deprecated, clean up the remaining plugin-related code:
- Remove the Plugin field from rosettaRunner
- Remove the -rosetta-plugin CLI flag
- Consider removing the hashicorp/go-plugin dependency if unused
🔗 Analysis chain
Line range hint
68-86: Verify removal of plugin argument impact.The
--pluginargument has been removed from the startup arguments, but thePluginfield still exists in the struct. This inconsistency needs to be addressed:
- If plugins are no longer supported, consider removing the
Pluginfield- If plugins are still supported, clarify why the argument is no longer needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for plugin usage and potential inconsistencies echo "Checking for plugin field usage..." ast-grep --pattern 'r.Plugin' echo "Checking for plugin-related test cases..." rg -i "plugin" "tests/systemtests"Length of output: 1055
Script:
#!/bin/bash # Check for recent changes and alternative plugin configuration # Check git history for plugin-related changes git log -n 5 --oneline -- tests/systemtests/rosetta.go # Look for potential new plugin configuration mechanisms rg -i "plugin.*config" tests/systemtests rg -i "loadplugin|load.*plugin" tests/systemtests # Check if plugin is used elsewhere in the start method ast-grep --pattern 'func (r *rosettaRunner) start(t *testing.T) { $$$ }'Length of output: 1491
🧹 Nitpick comments (1)
tests/systemtests/construction_test.go (1)
88-113: Add test cleanup and enhance assertions.The test function could be improved in the following areas:
- Add cleanup for created test accounts to prevent state pollution between tests
- Add assertions to verify the format and content of the unsigned transaction
Consider adding:
func TestPayloads(t *testing.T) { + t.Cleanup(func() { + sut.ResetChain(t) + }) // ... existing code ... assert.NotEmpty(t, gjson.GetBytes(res, "unsigned_transaction")) + // Verify unsigned transaction format + unsignedTx := gjson.GetBytes(res, "unsigned_transaction").String() + assert.True(t, strings.HasPrefix(unsignedTx, "{"), "unsigned transaction should be a JSON object") + assert.Contains(t, unsignedTx, "body", "unsigned transaction should contain a transaction body") + assert.Contains(t, unsignedTx, "auth_info", "unsigned transaction should contain auth info") assert.Equal(t, gjson.GetBytes(res, "payloads.0.address").String(), addr) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
tests/systemtests/accounts_test.go(1 hunks)tests/systemtests/block_test.go(1 hunks)tests/systemtests/construction_test.go(3 hunks)tests/systemtests/go.mod(1 hunks)tests/systemtests/main_test.go(1 hunks)tests/systemtests/mempool_test.go(1 hunks)tests/systemtests/network_test.go(1 hunks)tests/systemtests/rest_client.go(2 hunks)tests/systemtests/rosetta.go(2 hunks)tests/systemtests/test_runner.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/systemtests/go.mod
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/systemtests/main_test.go
- tests/systemtests/mempool_test.go
- tests/systemtests/accounts_test.go
- tests/systemtests/block_test.go
- tests/systemtests/test_runner.go
- tests/systemtests/network_test.go
- tests/systemtests/rest_client.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: rosetta-cli-test
🔇 Additional comments (7)
tests/systemtests/rosetta.go (3)
Line range hint
48-66: LGTM! Constructor changes align with struct modifications.The removal of the
offlineparameter fromnewRosettaRunneris consistent with the struct changes. The initialization values and defaults are appropriate.
1-1: Verify package name consistency across test files.The package name has been changed from
systemteststorossettaSystemTests. Let's ensure this change is consistent across all test files to prevent import issues.✅ Verification successful
Package name change is consistent across all test files
The package name has been successfully changed from
systemteststorossettaSystemTestsacross all files in the test suite, maintaining consistency throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old package name # and verify consistency of the new package name across test files echo "Checking for old package name references..." rg -l "package systemtests" "tests/systemtests" echo "Verifying new package name usage..." rg -l "package rossettaSystemTests" "tests/systemtests"Length of output: 581
Line range hint
28-47: Verify impact of removing offline functionality.The
Offlinefield has been removed from therosettaRunnerstruct. Please ensure that:
- All test cases that previously used offline mode have been updated
- Documentation reflects this change in functionality
- No remaining code paths assume the existence of offline mode
✅ Verification successful
Offline mode removal appears clean and complete
The verification shows no remaining references to offline functionality in tests, configuration, or initialization code. The
rosettaRunnerstruct's current state is consistent, with no lingering dependencies on the removedOfflinefield.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to offline functionality echo "Checking for offline mode references in tests..." rg -i "offline" "tests/systemtests" echo "Checking for offline mode references in documentation..." fd -e md | xargs rg -i "offline"Length of output: 296
Script:
#!/bin/bash # Check git history and code patterns # Look for rosetta configuration and initialization ast-grep --pattern 'rosettaRunner{$$$}' # Search for command flags and configuration rg -i "flag\.(StringVar|Bool|String|BoolVar).*offline" tests/systemtests # Look for test setup patterns rg -A 5 "func.*Test.*Rosetta" tests/systemtestsLength of output: 1214
tests/systemtests/construction_test.go (4)
3-3: Fix package name typo.The package name contains a typo: "rosseta" should be "rosetta".
-package rossettaSystemTests +package rosettaSystemTests
34-35: LGTM! Good improvement in address handling.The change properly uses the Bech32 codec for address conversion, which is the correct way to handle Cosmos addresses.
109-109: Avoid hardcoded account numbers.The test uses a hardcoded account number (1) which could make the test fragile. Instead, query the account number for the address being used.
- res, err := rosettaRest.constructionPayloads(`"signer_data":[{"account_number":1, "sequence": 0}]`, hexPk, op) + accNum := cli.QueryAccountNumber(addr) + res, err := rosettaRest.constructionPayloads(fmt.Sprintf(`"signer_data":[{"account_number":%d, "sequence": 0}]`, accNum), hexPk, op)
111-112: Add test coverage for the signature retrieval bug fix.According to the PR objectives, this change fixes a bug where the endpoint was retrieving unsigned transaction signatures instead of signers. However, the test doesn't explicitly verify this fix.
Consider adding assertions to verify that:
- The response contains signer information instead of signatures
- The unsigned transaction doesn't contain signatures
assert.NotEmpty(t, gjson.GetBytes(res, "unsigned_transaction")) + // Verify that unsigned transaction doesn't contain signatures + assert.False(t, gjson.GetBytes(res, "unsigned_transaction").Get("signatures").Exists(), + "unsigned transaction should not contain signatures") + // Verify that we get signer info instead of signatures + assert.True(t, gjson.GetBytes(res, "payloads.0.signature_type").Exists(), + "payload should contain signature type for signer") assert.Equal(t, gjson.GetBytes(res, "payloads.0.address").String(), addr)
This PR fixes a bug in the /construction/payloads endpoint where it was retrieving unsigned transaction signatures instead of signers.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Testing
rossettaSystemTests.Chores