feat(sdk)!: make document state transition entropy optional, will do a replace if revision is not 1. #2658
Conversation
WalkthroughThe Changes
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
QuantumExplorer
changed the title
![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
🧹 Nitpick comments (2)
packages/rs-sdk/src/platform/transition/put_document.rs (2)
70-72: Simplify the revision check for better readability and safety.The current revision check could be simplified and made more readable:
- let transition = if self.revision().is_some() - && self.revision().unwrap() != INITIAL_REVISION - { + let transition = if matches!(self.revision(), Some(revision) if revision != INITIAL_REVISION) {This approach avoids calling
unwrap()and is more idiomatic Rust.
70-111: Consider refactoring for improved maintainability.The method has grown complex with the addition of creation vs replacement logic. Consider extracting the branching logic into separate helper methods:
async fn put_to_platform(...) -> Result<StateTransition, Error> { let new_identity_contract_nonce = // existing logic... let transition = if self.is_replacement_scenario() { self.create_replacement_transition(/* params */).await? } else { self.create_creation_transition(/* params */).await? }; transition.broadcast(sdk, Some(settings)).await?; Ok(transition) }This would improve readability and make the code easier to test and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-sdk/src/platform/transition/put_document.rs(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (3)
packages/rs-sdk/src/platform/transition/put_document.rs (3)
5-6: LGTM! Import additions are appropriate.The added imports for random number generation and the
INITIAL_REVISIONconstant are necessary for the new optional entropy functionality.Also applies to: 9-9
54-54: LGTM! Implementation signatures correctly match trait updates.The implementation method signatures are updated consistently with the trait definition changes.
Also applies to: 122-122
85-98: LGTM! Entropy generation logic is secure and correct.The entropy generation logic properly handles the optional parameter:
- Uses secure
StdRng::from_entropy()for random generation- Correctly generates document ID when entropy is created
- Properly handles document immutability through cloning
The implementation is functionally sound and follows security best practices.
|
I would include in the title that we are also making put_document do Replace instead of Create if not initial revision |
QuantumExplorer
changed the title
QuantumExplorer
deleted the
featoptional-document-state-transition-entropy
branch
Issue being fixed or feature implemented
This update allows the
document_state_transition_entropyparameter to be optional, improving flexibility in document state transitions.What was done?
document_state_transition_entropyfrom a required[u8; 32]array to anOption<[u8; 32]>.How Has This Been Tested?
The changes were tested through existing unit tests that cover document state transitions, ensuring that both the creation and replacement scenarios function correctly with and without the entropy parameter.
Breaking Changes
None
Checklist
For repository code-owners and collaborators only
Summary by CodeRabbit