Skip to content

chore: use io-ts to parse vault keycard box + update keycard formatting and QR headers#9225

Open
s84krish wants to merge 1 commit into
masterfrom
WCN-1193-followups
Open

chore: use io-ts to parse vault keycard box + update keycard formatting and QR headers#9225
s84krish wants to merge 1 commit into
masterfrom
WCN-1193-followups

Conversation

@s84krish

@s84krish s84krish commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Ticket: WCN-1193

Description

This PR addresses followups from #9209 (comment) as well as makes improvements to the formatting and copy of vault keycards. Also includes a new part-based encoding for the QRs. Mock below:

1/2|{"secp256k1Multisig":"{\"v\":2,\"m\":65536,\"t\":3,\"p\":4,\"salt\":\"ABleiJ/ySLZvoaYswbRzTg==\",\"iv\":\"VmwUpGXO2LJ1OSnC\",\"ct\":\"NBRX+x21OL7RkrH7yDjdX6hk9+f55zadWqNFX2/xvo9lapluoeZcOxifnplHH+YkKKTkCJYK74695iry0Ch2RMChYpjXlYWK2VfhWiAC24CRtBc8e8HEyNWnfnV7Q5UJlwbb8Wc6zBwlm+UC2gWQr0k04zKKJS40w2FK9ywOiQ==\"}","ecdsaMpc":"{\"v\":2,...(DKLS reduced share ciphertext)...","eddsaMpc":"{\"v\":2,... [truncated mid-value where the 1500-char split falls]
2/2|tainZcXm355bbrxTaoULbjqKJE5XiLVyQRjygnImY1ruxsgTXo=\"}","eddsaMpc":"{\"v\":2,...}","ed25519Multisig":"{\"v\":2,...\"ct\":\"Dtt5R7Q8...==\"}"}

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Generated PDFs locally for vault and wallet keycards using mock data.
vault-keycard.pdf
keycard-ed25519Multisig.pdf

@linear-code

linear-code Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

WCN-1193

@s84krish s84krish force-pushed the WCN-1193-followups branch 2 times, most recently from e4495b2 to 5cde131 Compare July 9, 2026 21:05
@s84krish s84krish changed the title chore: use io-ts to parse vault keycard box + update keycard formatting chore: use io-ts to parse vault keycard box + update keycard formatting and QR headers Jul 9, 2026
@s84krish

s84krish commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@claude

@BitGo BitGo deleted a comment from github-actions Bot Jul 9, 2026
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @s84krish's task —— View job


Code Review Complete ✅

Todo List:

  • Read and analyze all changed files
  • Review io-ts implementation for vault keycard parsing
  • Examine QR data generation and part-based encoding
  • Check keycard formatting and drawing updates
  • Verify type safety and error handling
  • Provide comprehensive review feedback

Review Summary

This PR successfully addresses the followups from the previous discussion and implements several improvements to vault keycard generation. The changes are well-structured and maintain backward compatibility while adding the new part-based QR encoding.

Strengths

1. io-ts Integration (parseKeycard.ts:22-45)

The addition of io-ts for vault keycard box parsing is excellent:

const VaultKeycardRootsCodec: t.Type<VaultKeycardRoots> = t.type({
  secp256k1Multisig: t.string,
  ecdsaMpc: t.string,
  eddsaMpc: t.string,
  ed25519Multisig: t.string,
});
  • Type Safety: Ensures all four roots are present and string-typed
  • Error Handling: Clear error messages via PathReporter
  • Validation: Runtime validation that the JSON structure is correct

2. Part-Based QR Encoding (drawKeycard.ts:60-87)

The new QR fragment system is well-designed:

function encodeQrCodePart(fragment: string, index: number, total: number): string {
  return total > 1 ? `${index + 1}/${total}|${fragment}` : fragment;
}
  • Smart Headers: Only adds headers when fragmentation occurs (total > 1)
  • Safe Delimiter: Uses | which won't conflict with base64/JSON/base58 content
  • Clear Contract: Well-documented reassembly instructions for consumers

3. Improved Keycard Formatting (drawKeycard.ts:250-259)

Better handling of multi-part QR codes:

  • Contextual Notes: Adds helpful "put all Parts together" guidance
  • Layout Management: Proper positioning to avoid overlapping the data column
  • Visual Clarity: Distinguishes parts with "Part N" labels

4. Comprehensive Testing (vaultQrData.ts)

Strong test coverage that validates:

  • Round-trip parsing: Box → JSON → decrypt cycle works correctly
  • Error handling: Malformed data properly rejected
  • Real-world sizes: Uses actual root key lengths for realistic testing

🔍 Areas for Consideration

1. Dependency Addition (package.json:40-41)

"io-ts": "npm:@bitgo-forks/io-ts@2.1.4",
"io-ts-types": "^0.5.19",

Question: What necessitated the forked version of io-ts? Consider documenting the reason for future maintainers.

2. QR Part Headers Implementation (drawKeycard.ts:241)

const payload = useQrPartHeaders ? encodeQrCodePart(keys[i], i, keys.length) : keys[i];

Observation: The feature is opt-in via useQrPartHeaders flag, which is good for backward compatibility. The vault keycard uses this (index.ts:69) while regular wallet keycards don't.

3. PDF Line Parsing Robustness (parseKeycard.ts:69-75)

The normalization logic handles "Part N" labels well:

.filter((line) => !/^Part\s+\d+$/i.test(line.trim()))
.replace(/\s*Part\s+\d+\s*/gi, '')

Good: Handles both standalone labels and embedded labels from PDF text merging.

📋 Minor Suggestions

1. Type Export (types.ts:77)

The VaultKeycardRoots type is well-defined. Consider if the parsing function should be exported alongside for external consumers.

2. Error Message Consistency (parseKeycard.ts:42)

The error includes "parseVaultKeycardBox:" prefix which is helpful for debugging. This pattern could be extended to other parsing functions if not already present.

3. Magic Number Documentation (drawKeycard.ts:30)

export const QRBinaryMaxLength = 1500;

The comment explains the rationale well. The 1500-character limit balances QR density with scanning reliability.

🎯 Overall Assessment

Score: 9/10

This PR demonstrates excellent software engineering practices:

  • Type Safety: Proper io-ts integration with runtime validation
  • Backward Compatibility: Opt-in features that don't break existing functionality
  • Documentation: Clear comments explaining the part-based encoding contract
  • Testing: Comprehensive tests covering edge cases and real-world scenarios
  • Error Handling: Thoughtful error messages and validation

The implementation is production-ready and addresses the stated requirements effectively. The part-based QR encoding is particularly well-designed and will significantly improve the user experience for vault keycard recovery scenarios.


Job Run: View Logs

* does not affect PDF-based recovery.
*/
function encodeQrCodePart(fragment: string, index: number, total: number): string {
return total > 1 ? `${index + 1}/${total}|${fragment}` : fragment;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering what the best way to do this parts encoding would be - if it is even necessary.

@s84krish s84krish marked this pull request as ready for review July 9, 2026 21:34
@s84krish s84krish requested review from a team as code owners July 9, 2026 21:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the key-card module’s vault keycard UX and data handling by (1) switching vault box parsing to io-ts validation, (2) updating vault-specific Box D wording/copy, and (3) introducing QR “part headers” for split vault QR payloads while refining the PDF layout around multipart QRs.

Changes:

  • Add io-ts-based decoding for parseVaultKeycardBox, and adjust unit tests accordingly.
  • Update vault keycard wording (Box D title/description and “vault password” language) and plumb vault/wallet entity wording through passcode QR generation.
  • Add opt-in QR fragment part headers (enabled for vault keycards) and reposition multipart notes under the QR column with improved row-height calculation.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
modules/key-card/test/unit/vaultQrData.ts Updates vault QR data expectations (vault wording) and loosens malformed box parsing assertions; adds a “well-formed box” decode check.
modules/key-card/src/types.ts Adds useQrPartHeaders option and clarifies page break behavior docs.
modules/key-card/src/parseKeycard.ts Switches parseVaultKeycardBox to io-ts validation and updates Box D title matching to support vault wording.
modules/key-card/src/index.ts Enables QR part headers for vault keycard generation.
modules/key-card/src/generateQrData.ts Adds wallet/vault entity wording to Box D generation; updates vault box descriptions to “vault password”.
modules/key-card/src/drawKeycard.ts Implements QR part headers (opt-in) and adjusts multipart QR layout/note placement and row-height calculations.
modules/key-card/package.json Adds runtime deps needed for io-ts decoding (fp-ts, io-ts, io-ts-types).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread modules/key-card/src/drawKeycard.ts
Comment thread modules/key-card/src/drawKeycard.ts Outdated
Comment thread modules/key-card/src/parseKeycard.ts Outdated
@s84krish s84krish force-pushed the WCN-1193-followups branch from 5cde131 to f8dc5c4 Compare July 9, 2026 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants