fix(drive): mint to self if allowed and no destination id set #2508
Conversation
WalkthroughThe pull request refactors the Changes
Possibly related PRs
Suggested Reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (
|
![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/batch/batched_transition/token_transition/token_mint_transition_action/v0/transformer.rs (1)
5-5: Consider removing unused importThe
DestinationIdentityForTokenMintingNotSetErrorimport is now unused since the code no longer throws this error. Removing it would clean up the imports.- use dpp::consensus::basic::token::{ChoosingTokenMintRecipientNotAllowedError, DestinationIdentityForTokenMintingNotSetError}; + use dpp::consensus::basic::token::ChoosingTokenMintRecipientNotAllowedError;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_mint_transition_action/v0/transformer.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Check each feature
- 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
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (3)
packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_mint_transition_action/v0/transformer.rs (3)
146-161: Approved: Now allows fallback to self-minting when no destination is specifiedThis change properly implements the PR objective of allowing tokens to be minted to the owner (self) when no specific destination is set. By returning
owner_idwhen no destination identity is found, the code eliminates the unnecessary error that was previously thrown.
301-316: Approved: Consistent implementation in the borrowed function variantThe same change has been correctly applied to the borrowed function variant, ensuring consistency across both implementations. This allows for self-minting when no destination identity is specified.
119-144:❓ Verification inconclusive
Verification: Confirm error path is still properly covered by tests
The code still correctly prevents users from specifying a destination when not allowed by the token configuration. Since this PR changes related error handling, it's worth verifying that tests still cover this error path properly.
🏁 Script executed:
#!/bin/bash # Search for tests that verify the ChoosingTokenMintRecipientNotAllowedError handling echo "Searching for tests related to ChoosingTokenMintRecipientNotAllowedError..." rg -A 5 "ChoosingTokenMintRecipientNotAllowedError" --glob "**/*test*"Length of output: 231
Action Required: Confirm test coverage for error path
The code correctly returns a
ChoosingTokenMintRecipientNotAllowedErrorwhen a destination is specified against token configuration rules. However, our automated search did not locate any test cases explicitly referencingChoosingTokenMintRecipientNotAllowedError. Please manually verify that this error path is exercised in your tests. If tests are missing or incomplete, consider adding scenarios to cover this validation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/mod.rs (2)
406-497: Test name no longer matches behaviorThe test name
test_token_mint_by_owner_no_destination_causes_errorimplies that an error is expected, but the implementation now checks for successful execution when no destination is provided. Consider renaming the test to better reflect its current behavior.-fn test_token_mint_by_owner_no_destination_causes_error() { +fn test_token_mint_by_owner_no_destination_mints_to_self() {
700-795: Test name no longer matches behaviorSimilar to the previous test, this name
test_token_mint_by_owned_id_no_destination_causes_errorimplies an error is expected, but the implementation now checks for successful execution.-fn test_token_mint_by_owned_id_no_destination_causes_error() { +fn test_token_mint_by_owned_id_no_destination_mints_to_self() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/mod.rs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Formatting
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Unused dependencies
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
🔇 Additional comments (6)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/mod.rs (6)
430-431: Good addition to ensure accurate verificationCapturing the base supply before minting allows for precise verification of the token balances after the operation, ensuring the test properly validates the new self-minting behavior.
474-475: Successfully implements the intended behavior changeThe assertion has been appropriately updated to check for successful execution instead of the previous error. This properly tests that token minting works when no destination ID is provided.
484-496: Good verification of token balanceThis addition properly verifies that the token was minted to the owner's account when no destination was specified, confirming the new self-minting behavior works as expected.
728-729: Good addition to ensure accurate verificationSimilar to the previous test case, capturing the base supply allows for precise verification of token balances after the minting operation.
772-773: Successfully implements the intended behavior changeThe assertion has been appropriately updated to check for successful execution instead of the previous error, properly testing the new self-minting behavior.
782-796: Good verification of token balanceThis addition properly verifies that the token was minted to the owner's account when no destination was specified in the no-recipient minting scenario, confirming the feature works across different configuration settings.
Issue being fixed or feature implemented
When minting, in the case where no default destination identity is set, and no recipient is set, we should mint to self if allowed. However, we were preventing this and instead returning
BasicError::DestinationIdentityForTokenMintingNotSetError, which actually is a useless error, because even if destination identities are allowed to be set, it isn't required.What was done?
Mint to self if the contract allows that identity to mint, no recipient is set, and no default recipient is set.
How Has This Been Tested?
DET
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Refactor
Tests