fix: Update default cryptographic algorithm in Wallet class methods
#770
Conversation
WalkthroughThe pull request introduces several significant updates to the project. It adds the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
xrpl/wallet/main.py (1)
218-219: Enhance documentation with compatibility note.While the added documentation link is valuable, consider adding a note about compatibility considerations when changing from the previous default (SECP256K1) to ED25519.
Consider updating the docstring to include:
The default is ED25519. Docs: https://xrpl.org/docs/concepts/accounts/cryptographic-keys#ed25519-key-derivation + Note: If you're upgrading from a previous version that used SECP256K1 as + the default, ensure your code explicitly specifies the algorithm if + SECP256K1 is required.CHANGELOG.md (1)
95-95: Remove empty line.There's an unnecessary empty line with a dash that should be removed.
Apply this diff to remove the empty line:
--
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.md(2 hunks)tests/unit/asyn/wallet/test_main.py(1 hunks)xrpl/wallet/main.py(4 hunks)
🔇 Additional comments (5)
xrpl/wallet/main.py (3)
72-74: LGTM: Clear and appropriate comment style.
The comment effectively emphasizes the security-critical nature of the seed value while using the appropriate comment style for an internal implementation detail.
91-91: LGTM: Well-structured error handling enhancement.
The error handling improvement properly chains exceptions and provides comprehensive context including the seed, algorithm, and original error message, which will greatly aid in debugging.
207-207: Verify impact of changing the default algorithm.
The change to ED25519 as default aligns with the PR objective, but we should verify the impact on existing code that relies on the default behavior.
Let's check for any direct usage of this method without explicit algorithm specification:
✅ Verification successful
Default algorithm change to ED25519 is safe to proceed
The codebase analysis shows that:
- All direct usages of
from_secret_numbersare in test files - Tests explicitly cover both algorithms (ED25519 and SECP256K1)
- The change is properly documented in CHANGELOG.md
- No production code relies on the default behavior
The change is well-tested and doesn't introduce breaking changes in existing code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to from_secret_numbers that might be affected
rg "from_secret_numbers\([^)]*\)" --type python
# Search for tests that might need updating
rg "test.*from_secret_numbers" --type python
Length of output: 158
Script:
#!/bin/bash
# Search for calls to from_secret_numbers without file type flag
rg "from_secret_numbers\([^)]*\)"
# Also search for test files containing from_secret_numbers
rg "test.*from_secret_numbers"
# Search for any imports or references to from_secret_numbers
rg "from_secret_numbers"
Length of output: 3037
CHANGELOG.md (1)
Line range hint 1-24: Verify categorization of the change.
The PR objectives describe this as a bug fix that ensures consistency across all methods within the Wallet class, following up on changes made in PR #748. However, it's currently categorized as a breaking change in the changelog. Since version 2.0.0 already established ED25519 as the default signing algorithm, this change appears to be fixing an inconsistency rather than introducing a breaking change.
Please verify if this should indeed be a breaking change or if it's completing the transition that was already announced in version 2.0.0.
Let's verify the changes made in PR #748:
🧰 Tools
🪛 Markdownlint
13-13: Punctuation: ':'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
tests/unit/asyn/wallet/test_main.py (1)
320-320: LGTM! Test changes correctly reflect the new default algorithm.
The test assertions have been updated to expect ed25519 as the default algorithm for both string and array inputs to Wallet.from_secret_numbers. This aligns with the PR objective to set ed25519 as the default cryptographic algorithm.
Let's verify that this change is consistent with other default algorithm tests in the codebase:
Also applies to: 325-325
✅ Verification successful
Let me verify the implementation of these test methods to ensure they all consistently validate the ed25519 default algorithm.
Changes are consistent with default algorithm tests across the codebase
The verification confirms that all test methods using default algorithm consistently validate ed25519 as the expected algorithm. This includes tests for all wallet creation methods: create(), from_seed(), from_secret(), from_entropy(), and from_secret_numbers(). The changes in lines 320 and 325 align perfectly with this consistent pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all default algorithm tests expect ed25519
# Test: Search for test methods using default algorithm. Expect: All should use ed25519.
rg -A 2 "test_.*using_default_algorithm" tests/
Length of output: 2295
Script:
#!/bin/bash
# Search for _test_wallet_values calls in test methods to verify expected algorithm
rg -B 2 "_test_wallet_values.*ed25519" tests/unit/asyn/wallet/test_main.py
Length of output: 1526
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
96-96: Remove extra blank line.There's an unnecessary blank line that should be removed to maintain consistent formatting.
### BREAKING CHANGE - The default signing algorithm in the `Wallet` was changed from secp256k1 to ed25519 - ### Added:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.md(2 hunks)xrpl/wallet/main.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- xrpl/wallet/main.py
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~16-~16: Possible missing preposition found.
Context: ...and use 3.8 as new default. ### Fixed - Ensure consistent use of ED25519 as the defaul...
(AI_HYDRA_LEO_MISSING_TO)
🔇 Additional comments (1)
CHANGELOG.md (1)
15-16: LGTM! The changelog entry is well-positioned and clearly written.
The entry correctly documents this change under the "Fixed" section, appropriately noting that this ensures consistency with the ED25519 default established in v2.0.0.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~16-~16: Possible missing preposition found.
Context: ...and use 3.8 as new default. ### Fixed - Ensure consistent use of ED25519 as the defaul...
(AI_HYDRA_LEO_MISSING_TO)
|
Note to the reviewers: I've made an attempt to update the docs to better communicate these changes here: XRPLF/xrpl-dev-portal#2844 |
|
This is a breaking change. |
|
@ckeshava would need to put this into a major version release --- and move to the breaking change docs |
|
@justinr1234 I've moved the update-message into the "Breaking Changes" section. There is already one other breaking change as well. How do I indicate that the next release needs to be a major version upgrade? Since we are removing support for Python 3.7, I suspect we need to do a major version upgrade anyway. |
High Level Overview of Change
This change is related to this PR: #748
I've addressed my comment (#748 (comment)) in the current PR.
@anissa-ripple @justinr1234 @dangell7 please review the changes in this PR
Context of Change
Use
ed25519as the default cryptographic algorithm inWallet.from_secret_numbersmethod. This ensures consistency amongst all the methods of theWalletclass.Type of Change
Did you update CHANGELOG.md?
Test Plan