Problem: unsuppored sign mode SIGN_MODE_TEXTUAL in bank transfer #1617
Conversation
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
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 #1617 +/- ##
===========================================
+ Coverage 17.73% 36.88% +19.14%
===========================================
Files 72 102 +30
Lines 5204 8055 +2851
===========================================
+ Hits 923 2971 +2048
- Misses 4158 4706 +548
- Partials 123 378 +255 |
mmsqe
requested review from
JayT106 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: 0
🧹 Outside diff range and nitpick comments (2)
integration_tests/test_basic.py (1)
990-998: LGTM: Good test for textual sign mode, with room for enhancement.The
test_textualfunction effectively tests the basic functionality of the "textual" sign mode for transactions. It performs a transfer and checks for a successful response.Consider enhancing this test by:
- Verifying the balance changes of both the sender and receiver after the transfer.
- Testing with different amounts and multiple transfers to ensure consistency.
- Adding a negative test case where the transaction should fail (e.g., insufficient funds).
Example of balance verification:
def test_textual(cronos): cli = cronos.cosmos_cli() sender = cli.address("validator") receiver = cli.address("signer2") amount = "1basetcro" # Check initial balances sender_initial = cli.balance(sender) receiver_initial = cli.balance(receiver) rsp = cli.transfer(sender, receiver, amount, sign_mode="textual") assert rsp["code"] == 0, rsp["raw_log"] # Verify balance changes sender_final = cli.balance(sender) receiver_final = cli.balance(receiver) assert sender_final == sender_initial - int(amount) assert receiver_final == receiver_initial + int(amount)app/app.go (1)
Line range hint
1143-1160: Adjust log levels in 'setAnteHandler' for clarityIn the
setAnteHandlerfunction,app.Logger().Erroris used to log messages about setting the ante handler, even when there is no error. Using theErrorlog level for normal operations can be misleading.Recommend changing the log level to
Infofor normal operation messages.Apply this diff to adjust the log levels:
if len(blacklist) > 0 { - app.Logger().Error("Setting ante handler with blacklist", "size", len(blacklist), "hash", hex.EncodeToString(h.Sum(nil))) + app.Logger().Info("Setting ante handler with blacklist", "size", len(blacklist), "hash", hex.EncodeToString(h.Sum(nil))) for _, addr := range blacklist { - app.Logger().Error("Blacklisted address", "address", addr) + app.Logger().Info("Blacklisted address", "address", addr) } } else { - app.Logger().Error("Setting ante handler without blacklist") + app.Logger().Info("Setting ante handler without blacklist") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- app/app.go (4 hunks)
- integration_tests/test_basic.py (1 hunks)
🔇 Additional comments (11)
integration_tests/test_basic.py (2)
988-997: LGTM: Well-structured test for multisig account functionality.The
test_multi_accfunction effectively tests the creation and basic operations of a multisig account. It covers creating the account, transferring funds, and verifying the account details, which are essential aspects of multisig functionality.
988-998: Summary: Valuable additions to the test suite.The new test functions
test_multi_accandtest_textualare valuable additions to the integration test suite. They cover important functionalities:
test_multi_acc: Tests the creation and basic operations of multisig accounts.test_textual: Verifies the functionality of the "textual" sign mode for transactions.These additions enhance the overall test coverage of the system. While both tests are well-implemented for basic functionality, there's potential for further enhancement, particularly in the
test_textualfunction, to cover more edge cases and verify the results more thoroughly.CHANGELOG.md (4)
18-18: Fix for unsupported sign mode in bank transferThis fix addresses an issue with the unsupported sign mode
SIGN_MODE_TEXTUALfor bank transfers. It's important to ensure that all supported sign modes are properly handled to maintain compatibility and prevent transaction failures.
Line range hint
20-24: Multiple improvements and bug fixes in v1.0.7This release includes several improvements and bug fixes:
- Performance optimization by reusing recovered sender addresses.
- Addition of static-linked binaries for Linux platforms.
- Introduction of PebbleDB backend.
- Bug fixes related to JSON-RPC APIs for legacy blocks and third-party fixes.
These changes should enhance performance, expand platform support, and resolve several issues. The addition of PebbleDB as a backend option could provide performance benefits depending on the use case.
🧰 Tools
🪛 Markdownlint
20-20: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
Line range hint
28-32: Bug fix for JSON-RPC APIs in v1.0.6This release focuses on backporting multiple JSON-RPC bug fixes from Ethermint. Ensuring the stability and correctness of JSON-RPC APIs is crucial for maintaining compatibility with Ethereum tooling and dApps.
🧰 Tools
🪛 Markdownlint
20-20: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
Line range hint
1-1185: Overall assessment of recent changesThe recent releases of Cronos (v1.0.6, v1.0.7, and the upcoming unreleased version) show a focus on:
- Improving compatibility and stability of Ethereum-related features (JSON-RPC APIs, EVM execution).
- Enhancing performance through optimizations and new backend options.
- Expanding platform support (e.g., static-linked binaries for Linux).
- Fixing critical issues related to transaction processing and state management.
These changes indicate an active development process with a strong emphasis on maintaining compatibility with the Ethereum ecosystem while improving the overall performance and stability of the Cronos blockchain.
For future releases, consider:
- Providing more detailed explanations of state machine breaking changes and their potential impact on users and developers.
- Including upgrade instructions or migration guides for significant changes.
- Grouping related changes together for better readability.
🧰 Tools
🪛 Markdownlint
20-20: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
app/app.go (5)
16-16: Approved: Import of 'slices' packageThe
slicespackage is imported and used appropriately when cloning the default sign modes.
66-66: Approved: Import of 'sigtypes' packageThe
sigtypespackage is imported and used correctly to include theSIGN_MODE_TEXTUALsign mode.
74-74: Approved: Import of 'txmodule' packageThe
txmodulepackage is imported and used appropriately for the coin metadata query function.
520-534: Enable 'SIGN_MODE_TEXTUAL' in transaction configurationThe code correctly enables the
SIGN_MODE_TEXTUALsign mode by cloning the default sign modes and appending the new sign mode. It then creates a new transaction configuration usingauthtx.NewTxConfigWithOptionswith the updated sign modes and the coin metadata query function.The error handling with
panic(err)is appropriate in this initialization context.
Line range hint
1002-1007: Updated 'setAnteHandler' to use the new 'txConfig'The code correctly passes the updated
txConfigwith the enabledSIGN_MODE_TEXTUALsign mode tosetAnteHandler. This ensures that the ante handler is aware of the new signing modes.
👮🏻👮🏻👮🏻 !!!! 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
New Features
Bug Fixes
Documentation