[SDK] buildFinalizeUnwrapCalldata + FinalizeUnwrapERC7984 explain/verify

[0xPrabh]

Description

Adds buildFinalizeUnwrapCalldata(requestId, cleartextAmount, decryptionProof) and FinalizeUnwrapERC7984 transaction support for ERC-7984 unshield phase-2.

• Encodes finalizeUnwrap(bytes32,uint64,bytes) with selector 0x5bb67a05
• Validates 32-byte requestId, cleartextAmount > 0 and ≤ uint64, non-empty decryptionProof
• Adds TransactionType.FinalizeUnwrapERC7984 with builder + classifyTransaction mapping
• Erc7984Token.verifyTransaction finalizeUnwrap branch (direct + sendMultiSig); optional amount match vs recipients
• Does not set gasLimit (WP owns gas)

Issue Number

CHALO-1157

Parent: CHALO-1135

Docs

• Generated using yarn docs

Tests

• Unit tests for buildFinalizeUnwrapCalldata / decode round-trip and validation in abstract-eth
• Shared FinalizeUnwrapERC7984 transaction builder suite (wired via sdk-coin-eth)
• verifyTransaction coverage for direct and sendMultiSig finalizeUnwrap shapes

Made with Cursor

[linear-code]

CHALO-1157

[0xPrabh]

@claude

[github-actions]

Claude finished @0xPrabh's task —— View job

---

Code Review Analysis

Since no specific request was provided, I'm performing a comprehensive code review of the PR changes.

Todo List:

• Read and understand the PR changes
• Analyze the implementation for correctness and best practices
• Check test coverage and quality
• Provide detailed review feedback
• Summary and recommendations

Code Review Findings

This PR implements ERC-7984 finalizeUnwrap functionality, completing the unshield phase-2 process. Here are my findings:

✅ Strengths

1. Comprehensive Implementation: The PR properly implements all necessary components:

   • Encoding/decoding functions in zamaUtils.ts
   • Transaction builder validation and type handling
   • Verification logic in erc7984Token.ts
   • Thorough test coverage

2. Proper Input Validation:

   • buildFinalizeUnwrapCalldata correctly validates 32-byte requestId, positive cleartextAmount ≤ uint64, and non-empty decryptionProof
   • Transaction builder includes dedicated validation methods for each parameter
   • ERC7984Token verification handles both direct and sendMultiSig transaction shapes

3. Security Best Practices:

   • Uses checksummed addresses via ethers.utils.getAddress()
   • Validates uint64 bounds using UINT64_MAX constant
   • Proper error handling with descriptive messages
   • Amount matching validation between calldata and transaction parameters

4. Code Organization:

   • Follows existing patterns and conventions
   • Consistent naming and structure with other ERC-7984 functions
   • Proper TypeScript typing throughout

🔍 Areas for Review

1. Method Selector Verification (zamaUtils.ts:72-74):

   export const finalizeUnwrapMethodId = addHexPrefix(
     EthereumAbi.methodID('finalizeUnwrap', [...finalizeUnwrapTypes]).toString('hex')
   );

   The comment states selector 0x5bb67a05 but should verify this matches the computed value.

2. Optional Amount Matching (erc7984Token.ts:459-464):

   const expectedAmount = txParams?.recipients?.[0]?.amount ?? txPrebuild.buildParams?.recipients?.[0]?.amount;
   if (expectedAmount !== undefined && String(expectedAmount) !== cleartextAmount) {

   This optional validation is good, but consider if there are cases where it should be required.

3. Gas Limit Handling: As noted in the description, gas limit is not set as "WP owns gas". Ensure this is consistently documented and handled across all ERC-7984 operations.

📋 Test Coverage Assessment

The tests are comprehensive and well-structured:

• Round-trip encoding/decoding validation
• Input validation edge cases
• Transaction classification
• Both successful and error scenarios
• Integration with existing test suites

🎯 Minor Suggestions

1. Consider adding a comment explaining why gas limit is not set in the calldata builders
2. The error messages are descriptive and helpful for debugging
3. The implementation correctly handles both direct calls and sendMultiSig wrapping

---

Todo List:

• Read and understand the PR changes
• Analyze the implementation for correctness and best practices
• Check test coverage and quality
• Provide detailed review feedback
• Summary and recommendations

Summary

This is a solid implementation that follows BitGoJS patterns and includes comprehensive validation, testing, and error handling. The code is production-ready and maintains consistency with existing ERC-7984 functionality. No blocking issues identified.

Recommendation: ✅ Approve - Ready for merge

---

Job Run: View Details