fix(platform): paying for a document action with tokens where tokens would be transferred to yourself as contract owner was breaking #2633
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Owner
participant ContractOwner
participant System
Owner->>System: Initiate document transition (create, delete, etc.)
alt Owner != ContractOwner
System->>ContractOwner: Push TokenTransfer operation
else Owner == ContractOwner
System-->>System: Skip TokenTransfer operation
end
System->>System: Validate token aggregated balance
System-->>Owner: Return result or error (CorruptedTokensNotBalanced)
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: 0
🧹 Nitpick comments (1)
packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_replace_transition.rs (1)
74-82: LGTM! Consistent pattern implementation across all document operations.The conditional check correctly prevents self-transfers during document replacement operations. The implementation maintains the established pattern across all document transition types, which ensures consistent behavior and excellent code maintainability.
Optional suggestion for future consideration: Since this exact pattern is repeated across multiple document transition files, consider extracting it into a helper function to reduce code duplication and ensure consistency. This could be addressed in a future refactoring effort.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/rs-drive-abci/src/error/execution.rs(1 hunks)packages/rs-drive-abci/src/execution/platform_events/tokens/validate_token_aggregated_balance/v0/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/creation.rs(3 hunks)packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_create_transition.rs(1 hunks)packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_delete_transition.rs(1 hunks)packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_purchase_transition.rs(1 hunks)packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_replace_transition.rs(1 hunks)packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_transfer_transition.rs(1 hunks)packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_update_price_transition.rs(1 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 (11)
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Formatting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
🔇 Additional comments (10)
packages/rs-drive-abci/src/error/execution.rs (1)
103-105: Well-implemented error variant addition.The new
CorruptedTokensNotBalancedvariant follows the established patterns and naming conventions. It properly separates token balance errors from credit balance errors, improving error specificity.packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_delete_transition.rs (1)
64-72: Effective prevention of redundant self-transfers.The conditional check correctly prevents token transfer operations when the owner and contract owner are identical. This optimization avoids unnecessary operations while maintaining the existing behavior for legitimate transfers.
packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_purchase_transition.rs (1)
87-95: Consistent implementation of self-transfer prevention.The conditional logic matches the pattern established in other document action files, ensuring consistent behavior across all document transitions. The implementation correctly prevents redundant token transfers while preserving legitimate transfer operations.
packages/rs-drive-abci/src/execution/platform_events/tokens/validate_token_aggregated_balance/v0/mod.rs (1)
24-25: Proper alignment with new token error variant.The error type change from
CorruptedCreditsNotBalancedtoCorruptedTokensNotBalancedcorrectly utilizes the new error variant and provides more accurate error context by referencing "tokens" instead of "credits" in the message.packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_update_price_transition.rs (1)
69-77: LGTM! Excellent optimization to prevent redundant self-transfers.The conditional check correctly prevents unnecessary token transfer operations when the document owner and contract owner are the same entity. This optimization improves performance while maintaining logical correctness, as transferring tokens to oneself would be a no-op operation.
packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_transfer_transition.rs (1)
76-84: LGTM! Consistent implementation of self-transfer prevention.The implementation follows the same pattern established in other document transition files, correctly preventing redundant token transfers when the owner and contract owner are identical. The consistency across all document operation types is excellent for maintainability.
packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_create_transition.rs (1)
73-81: LGTM! Correct implementation with minor style variation.The conditional check correctly prevents self-transfers during document creation. Note that the contract owner is accessed inline (
contract_fetch_info.contract.owner_id()) rather than being stored in a variable like in other files, but this is functionally equivalent and doesn't impact correctness.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/creation.rs (3)
678-680: LGTM! Token balance validation enhances test robustness.The addition of token balance validation after state transition processing is a good practice that ensures token consistency before committing the transaction.
823-825: LGTM! Consistent validation pattern applied.The token balance validation is consistently applied across different token operation scenarios, which is good for comprehensive testing.
861-2989: LGTM! Essential test coverage for self-payment scenario.This new test effectively validates the core PR objective - preventing redundant token transfers when the sender and recipient are the same entity. The test correctly verifies that the contract owner's token balance remains unchanged (15 tokens) when paying themselves for document creation, demonstrating that no actual transfer occurs in this scenario.
Key strengths:
- Comprehensive test setup following established patterns
- Proper assertion that token balance remains unchanged (
assert_eq!(token_balance, Some(15)))- Includes token balance validation for consistency checks
- Covers the edge case mentioned in the PR objectives
Issue being fixed or feature implemented
Paying for a document action with tokens where tokens would be transferred to yourself as contract owner was breaking.
What was done?
Updated document action transitions to check if the owner is the same as the sender before performing a token transfer. If they are the same, the transfer operation is skipped, but the document is still made.
How Has This Been Tested?
Added tests to ensure that token transfers are not executed when the sender and recipient are the same.
Breaking Changes
None
Checklist
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Tests