fix(sdk): match ItemWithSumItem in extract_balance_from_element
#2956
Conversation
✅ gRPC Query Coverage Report |
📝 WalkthroughWalkthroughA single file in the address synchronization module was updated to change how balance values are extracted from elements: the pattern now matches Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-sdk/src/platform/address_sync/mod.rs (1)
522-528: Fix broken test: Update to use ItemWithSumItem.The test creates an
Element::SumItembut the updated implementation (Line 463) now matchesItemWithSumIteminstead. This test will fail becauseSumItemfalls through to the catch-all pattern and returns 0, not 1000.🐛 Proposed fix for the test
#[test] fn test_extract_balance() { - let sum_item = Element::SumItem(1000, None); - assert_eq!(extract_balance_from_element(&sum_item), 1000); + let item_with_sum = Element::ItemWithSumItem(vec![], 1000, None); + assert_eq!(extract_balance_from_element(&item_with_sum), 1000); let item = Element::Item(vec![1, 2, 3], None); assert_eq!(extract_balance_from_element(&item), 0); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-sdk/src/platform/address_sync/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-sdk/src/platform/address_sync/mod.rs
🧠 Learnings (5)
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2201
File: packages/rs-platform-version/src/version/v2.rs:1186-1188
Timestamp: 2024-10-09T00:22:57.778Z
Learning: In the `IdentityTransitionVersions` structure within `packages/rs-platform-version/src/version/v2.rs`, the field `credit_withdrawal` does not need the `identity_` prefix since it is already encompassed within identity state transitions.
Applied to files:
packages/rs-sdk/src/platform/address_sync/mod.rs
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-sdk/src/platform/address_sync/mod.rs
📚 Learning: 2024-10-08T13:28:03.529Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-10-08T13:28:03.529Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
Applied to files:
packages/rs-sdk/src/platform/address_sync/mod.rs
📚 Learning: 2024-11-28T13:49:17.301Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2317
File: packages/rs-dapi-client/src/address_list.rs:175-180
Timestamp: 2024-11-28T13:49:17.301Z
Learning: In Rust code in `packages/rs-dapi-client/src/address_list.rs`, do not change the interface of deprecated methods like `add_uri`, even to fix potential panics.
Applied to files:
packages/rs-sdk/src/platform/address_sync/mod.rs
📚 Learning: 2025-02-03T23:38:33.414Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2450
File: packages/rs-drive/src/drive/tokens/distribution/fetch/pre_programmed_distributions/mod.rs:75-81
Timestamp: 2025-02-03T23:38:33.414Z
Learning: GroveDB's `as_sum_item_value()` method returns an `i64`, so sum items cannot exceed `i64::MAX` by design.
Applied to files:
packages/rs-sdk/src/platform/address_sync/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios-sim)
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios)
🔇 Additional comments (2)
packages/rs-sdk/src/platform/address_sync/mod.rs (2)
460-460: LGTM: Doc comment accurately reflects the implementation change.The updated documentation correctly describes that balances are stored as items with sum items.
463-463: LGTM: Pattern correctly extracts the sum value from ItemWithSumItem.The change from
SumItemtoItemWithSumItempattern correctly extracts the balance value from the second field, fixing the issue where the function was always returning 0.
| fn extract_balance_from_element(element: &Element) -> u64 { | ||
| match element { | ||
| Element::SumItem(value, _) => *value as u64, | ||
| Element::ItemWithSumItem(_, value, _) => *value as u64, |
There was a problem hiding this comment.
I don't understand how SumItem differs from ItemWithSumItem, and if it's correct to return 0 in that case, but maybe add some tracing::debug! log for _ ?
Issue being fixed or feature implemented
In the SDK we were matching
SumItems inextract_balance_from_elementbut address balances are stored asItemWithSumItems so the function always returned 0.What was done?
Replace
SumItemwithItemWithSumItemHow Has This Been Tested?
DET
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.