feat: format wallet event amounts as DASH#530
Conversation
Use `Amount` and `SignedAmount` for better readable logging output in wallet event descriptions.
📝 WalkthroughWalkthroughThe changes extend imports to include Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #530 +/- ##
=============================================
- Coverage 66.91% 66.88% -0.04%
=============================================
Files 313 313
Lines 64741 64748 +7
=============================================
- Hits 43321 43305 -16
- Misses 21420 21443 +23
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
key-wallet-manager/src/events.rs (2)
47-75: 🛠️ Refactor suggestion | 🟠 MajorPlease add regression tests for
description().This changes user-visible log strings and adds the new
lockedcomponent, but there is no adjacent#[cfg(test)]coverage pinning the new formatted output. A couple of focused tests here would prevent silent format regressions.As per coding guidelines, "**/*.rs: Write unit tests for new functionality in Rust code" and "Place unit tests alongside code with
#[cfg(test)]attribute."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/events.rs` around lines 47 - 75, Add unit tests for WalletEvent::description() under a #[cfg(test)] mod in the same file: create tests that construct a TransactionReceived and a BalanceUpdated (including the new locked field) and assert the returned String matches the expected formatted output (checking txid and SignedAmount::from_sat formatting for TransactionReceived and Amount::from_sat formatting for each field in BalanceUpdated). Ensure tests cover both positive and zero/edge values for locked to prevent regressions in the new component and use the exact variant constructors (WalletEvent::TransactionReceived and WalletEvent::BalanceUpdated) so the assertions will fail on any formatting changes.
31-41:⚠️ Potential issue | 🟠 MajorAdd unit tests for
WalletEvent::description()and document breaking API change.Adding
lockedtoWalletEvent::BalanceUpdatedis a breaking API change—downstream exhaustive pattern matches and direct constructors without all four balance fields will no longer compile. Confirm this change is acceptable in your versioning strategy, or document migration guidance in release notes.Additionally, per coding guidelines, add
#[cfg(test)]tests alongside thedescription()method to validate the formatted output includes thelockedfield:#[cfg(test)] mod tests { use super::*; #[test] fn test_balance_updated_description_includes_all_fields() { // Verify spendable, unconfirmed, immature, and locked appear in output } #[test] fn test_transaction_received_description_format() { // Verify SignedAmount formatting } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/events.rs` around lines 31 - 41, Update docs and add unit tests to account for the breaking API change from adding the locked field to WalletEvent::BalanceUpdated: update release notes or migration guidance to call out that exhaustive pattern matches and constructors must now include the new locked: u64 field. Also add #[cfg(test)] tests next to the description() impl: create tests verifying WalletEvent::BalanceUpdated::description() output contains spendable, unconfirmed, immature and locked values, and a test verifying WalletEvent::TransactionReceived (or the type using SignedAmount) description formats SignedAmount correctly; reference WalletEvent::BalanceUpdated, WalletEvent::description, and the SignedAmount formatting in the test names so future readers can find them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@key-wallet-manager/src/events.rs`:
- Around line 47-75: Add unit tests for WalletEvent::description() under a
#[cfg(test)] mod in the same file: create tests that construct a
TransactionReceived and a BalanceUpdated (including the new locked field) and
assert the returned String matches the expected formatted output (checking txid
and SignedAmount::from_sat formatting for TransactionReceived and
Amount::from_sat formatting for each field in BalanceUpdated). Ensure tests
cover both positive and zero/edge values for locked to prevent regressions in
the new component and use the exact variant constructors
(WalletEvent::TransactionReceived and WalletEvent::BalanceUpdated) so the
assertions will fail on any formatting changes.
- Around line 31-41: Update docs and add unit tests to account for the breaking
API change from adding the locked field to WalletEvent::BalanceUpdated: update
release notes or migration guidance to call out that exhaustive pattern matches
and constructors must now include the new locked: u64 field. Also add
#[cfg(test)] tests next to the description() impl: create tests verifying
WalletEvent::BalanceUpdated::description() output contains spendable,
unconfirmed, immature and locked values, and a test verifying
WalletEvent::TransactionReceived (or the type using SignedAmount) description
formats SignedAmount correctly; reference WalletEvent::BalanceUpdated,
WalletEvent::description, and the SignedAmount formatting in the test names so
future readers can find them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a19e78d8-9fdd-4d5f-acbb-284e5e80535f
📒 Files selected for processing (1)
key-wallet-manager/src/events.rs
1 participant
Use
AmountandSignedAmountfor better readable logging output in wallet event descriptions.Summary by CodeRabbit