fix(sdk): using incorrect proof result in transfers #2379
Conversation
|
Warning Rate limit exceeded@pauldelucia has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
pauldelucia
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
🧹 Outside diff range and nitpick comments (1)
packages/rs-sdk/src/platform/transition/transfer.rs (1)
65-66: LGTM! Consider enhancing error messages.The change from
VerifiedPartialIdentitytoVerifiedBalanceTransfercorrectly aligns with the transfer operation semantics and fixes the proof result handling issue. The implementation properly accesses the balance from the sender identity.Consider making the error messages more specific:
- _ => Err(Error::DapiClientError("proved a non identity".to_string())), + _ => Err(Error::DapiClientError("expected VerifiedBalanceTransfer result but got a different proof result".to_string())),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/rs-sdk/src/platform/transition/transfer.rs(1 hunks)
🔇 Additional comments (1)
packages/rs-sdk/src/platform/transition/transfer.rs (1)
65-66: Verify complete migration to VerifiedBalanceTransfer.
Let's ensure no other transfer-related code still expects VerifiedPartialIdentity.
✅ Verification successful
Migration to VerifiedBalanceTransfer is correctly implemented
The verification shows that:
VerifiedBalanceTransferis properly used in transfer.rs for balance transfersVerifiedPartialIdentityis only used in other contexts (withdraw and top-up operations) where it's the appropriate variant- The enum definition in rs-dpp confirms
VerifiedBalanceTransfertakes twoPartialIdentityparameters for sender and recipient
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of VerifiedPartialIdentity in transfer-related code
rg -A 5 "VerifiedPartialIdentity.*transfer"
# Search for all uses of StateTransitionProofResult to ensure consistent handling
ast-grep --pattern 'match $_ { StateTransitionProofResult::$$$_ }'
Length of output: 114
Script:
#!/bin/bash
# Let's try a broader search for StateTransitionProofResult variants
rg -A 5 "StateTransitionProofResult::"
# Also search for any VerifiedPartialIdentity mentions
rg "VerifiedPartialIdentity"
# Look for transfer-related state transitions
rg -A 5 "BalanceTransfer"
Length of output: 12481
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/rs-sdk/src/platform/transition/transfer.rs (2)
71-71: Clarify the error message for unexpected result typeThe error message
"proved a non identity"may be unclear or contain a typo. It could be confusing for users encountering this error.Consider revising the error message to be more descriptive:
- _ => Err(Error::DapiClientError("proved a non identity".to_string())), + _ => Err(Error::DapiClientError("expected a VerifiedBalanceTransfer result".to_string())),
67-69: Enhance error handling when balance is missingWhen the
sender.balanceisNone, the error message"expected an identity balance after transfer"might not provide sufficient context.Consider adding more details to the error message to aid in debugging:
- sender.balance.ok_or(Error::DapiClientError( - "expected an identity balance after transfer".to_string(), - )) + sender.balance.ok_or(Error::DapiClientError(format!( + "expected an identity balance after transfer for identity: {}", + sender.id() + )))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/rs-sdk/src/platform/transition/transfer.rs(1 hunks)
🔇 Additional comments (1)
packages/rs-sdk/src/platform/transition/transfer.rs (1)
65-72:
Undefined variable result used in match statement
The variable result used in the match statement is not defined in this scope. Currently, identity is assigned from state_transition.broadcast_and_wait(sdk, settings).await?, but result is not declared.
To fix this issue, assign the result to result instead of identity:
- let identity: PartialIdentity = state_transition.broadcast_and_wait(sdk, settings).await?;
+ let result = state_transition.broadcast_and_wait(sdk, settings).await?;
match result {
StateTransitionProofResult::VerifiedBalanceTransfer(sender, _recipient) => {
sender.balance.ok_or(Error::DapiClientError(
"expected an identity balance after transfer".to_string(),
))
}
_ => Err(Error::DapiClientError("received unexpected proof result".to_string())),
}Alternatively, if broadcast_and_wait returns a PartialIdentity, ensure that you're using the correct variable and types in your match statement.
Run the following script to verify the return type of broadcast_and_wait:
✅ Verification successful
Let me gather more information about the implementation and usage of this method.
Let me try a different search strategy to understand the context better.
The review comment is correct - there's an undefined variable result in the code
Looking at the complete implementation in transfer.rs, we can see that broadcast_and_wait returns a PartialIdentity which is assigned to the identity variable, but then the code tries to match on an undefined result variable. The review's suggested fix to assign to result instead of identity is the correct approach since the match pattern expects a StateTransitionProofResult.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the return type of `broadcast_and_wait` method.
# Expected: Find the definition of `broadcast_and_wait` and check its return type.
rg -A 5 'fn broadcast_and_wait' packages/rs-sdk/src/platform/transition/broadcast.rs
Length of output: 484
Script:
#!/bin/bash
# Check the implementation of StateTransitionProofResult enum
rg -A 10 "enum StateTransitionProofResult" packages/rs-sdk/src/
# Check implementations of TryFrom<StateTransitionProofResult> for PartialIdentity
ast-grep --pattern 'impl TryFrom<StateTransitionProofResult> for PartialIdentity'
# Find where broadcast_and_wait is called with PartialIdentity
rg -B 5 "broadcast_and_wait.*PartialIdentity" packages/rs-sdk/src/
Length of output: 218
Script:
#!/bin/bash
# Find the file containing StateTransitionProofResult definition
fd -e rs | xargs rg "StateTransitionProofResult"
# Look for the implementation of the transfer module
rg -A 10 "impl Transfer" packages/rs-sdk/src/platform/transition/transfer.rs
# Check the complete transfer.rs file to understand the context
cat packages/rs-sdk/src/platform/transition/transfer.rs
Length of output: 6161
Issue being fixed or feature implemented
We were using the wrong proof result in transfers, so it would always return a proof error even though the transfer is successful
What was done?
How Has This Been Tested?
Untested
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
transfer_creditsfunctionality for improved balance transfer handling.Bug Fixes