fix(dapi): invalid proof for destroy frozen funds transition #2513
Conversation
WalkthroughThis pull request introduces changes to the handling of certain token transitions, specifically adding cases in the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)packages/dapi/lib/externalApis/drive/fetchProofForStateTransitionFactory.js (1)⏰ Context from checks skipped due to timeout of 90000ms (11)
🔇 Additional comments (2)
✨ 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/dapi/lib/externalApis/drive/fetchProofForStateTransitionFactory.js (1)
32-34: Consider consistency with other transition types.While this solution works, note that other transition types like
Burn,Mint, etc. use specific configuration methods (e.g.,keepsBurningHistory()) to determine history keeping. For consistency, consider asking the token configuration if it should keep history forDestroyFrozenFundstoo, rather than hardcodingtrue.If this is intentional and
DestroyFrozenFundsshould always keep history regardless of configuration, it would be helpful to add a comment explaining this design decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/dapi/lib/externalApis/drive/fetchProofForStateTransitionFactory.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/dapi/lib/externalApis/drive/fetchProofForStateTransitionFactory.js (1)
Learnt from: shumkov
PR: dashpay/platform#2449
File: packages/dapi/lib/externalApis/drive/fetchProofForStateTransitionFactory.js:110-112
Timestamp: 2025-03-25T08:03:56.379Z
Learning: The getRecipientId() method has different signatures for mint and transfer token transitions - mint requires tokenConfiguration parameter while transfer does not, due to their different logic and states.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (1)
packages/dapi/lib/externalApis/drive/fetchProofForStateTransitionFactory.js (1)
32-34: Correctly implemented solution for token transition history handling.This change addresses the proof verification issue identified in the PR objectives for the
DestroyFrozenFundstoken transition. By forcing this transition type to use the historical document proof path (returningtruefromkeepsHistory), it ensures that a proper proof will be generated rather than hitting the default error case in the switch statement.The implementation is elegant as it leverages the existing conditional structure in a minimally invasive way. The proof generation will now correctly follow the path at lines 106-119, generating document requests for token history.
QuantumExplorer
changed the title
github-project-automation
bot
moved this from In review / testing
to Done
in Platform team
Issue being fixed or feature implemented
Proof verification for destroy frozen funds transition is failing:
What was done?
How Has This Been Tested?
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit