-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update CHANGELOG.md #748
Update CHANGELOG.md #748
Conversation
Hello,
^^ Here are two different default-choices used in two different methods. |
WalkthroughThe pull request updates the Changes
Possibly related PRs
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
CHANGELOG.md
(1 hunks)
🔇 Additional comments (1)
CHANGELOG.md (1)
Line range hint 8-19
: LGTM! The Unreleased section follows changelog guidelines.
The changes are properly categorized and provide clear, meaningful information to users. The breaking change about Python 3.7 removal is well-documented with its rationale.
🧰 Tools
🪛 Markdownlint
95-95: Punctuation: ':'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
89-89: null
Bare URL used
(MD034, no-bare-urls)
### BREAKING CHANGE | ||
- The default signing algorithm in the `Wallet` was changed from secp256k1 to ed25519 | ||
- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Inconsistent default signing algorithm found in xrpl/wallet/main.py
The codebase shows inconsistency with the changelog's statement about ED25519 being the default signing algorithm:
- Most methods in
xrpl/wallet/main.py
useED25519
as default:generate_faucet_wallet
from_seed
from_entropy
- However,
from_secret_numbers
method still usesSECP256K1
as default with a comment referencing XUMM standard from December 2022
🔗 Analysis chain
Verify cryptographic algorithm consistency in the codebase.
While the changelog documents ED25519 as the new default signing algorithm, there appears to be inconsistency in its implementation. As noted in the PR comments, there's a mixture of SECP256K1 and ED25519 being used as defaults in xrpl/wallet/main.py
.
Let's verify the algorithm usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistent cryptographic algorithm usage
# Expected: ED25519 should be the default as per v2.0.0 changelog
# Search for algorithm-related code in wallet implementation
rg -A 5 "(?i)(secp256k1|ed25519)" "xrpl/wallet/main.py"
Length of output: 1831
High Level Overview of Change
Context of Change
Type of Change
Did you update CHANGELOG.md?
Test Plan
Summary by CodeRabbit
New Features
include_deleted
parameter to theledger_entry
request.Breaking Changes
Bug Fixes
FeeSettings
values from the latest validated ledger, removing hard-coded transaction cost.Documentation
CHANGELOG.md
to reflect notable changes and improvements across versions.