Skip to content

feat: filter safe key ceremonies by enabled root slots - #9534

Merged
danielpeng1 merged 1 commit into
masterfrom
WCN-1658/filter-enabled-root-slots
Aug 21, 2026
Merged

feat: filter safe key ceremonies by enabled root slots#9534
danielpeng1 merged 1 commit into
masterfrom
WCN-1658/filter-enabled-root-slots

Conversation

@danielpeng1

Copy link
Copy Markdown
Contributor

Gate SDK safe root-key ceremonies on the slots WP returns at initialize.

  • Thread enabledRootSlots from initializeSafe into createSafeKeys and generateSafe
  • Run only those slot ceremonies in parallel; omit the field and still run all 4 for older WP

Tests:

  • Decode enabledRootSlots on initialize
  • Subset enabledRootSlots skips the other ceremonies
  • generateSafe forwards enabledRootSlots into createSafeKeys

Ticket: WCN-1658

@danielpeng1 danielpeng1 self-assigned this Aug 20, 2026
@linear-code

linear-code Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

WCN-1658

@danielpeng1
danielpeng1 marked this pull request as ready for review August 20, 2026 19:24
@danielpeng1
danielpeng1 requested review from a team as code owners August 20, 2026 19:24
@danielpeng1

Copy link
Copy Markdown
Contributor Author

mostly a cherry-pick of #9374

@danielpeng1

Copy link
Copy Markdown
Contributor Author

@claude review

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @danielpeng1's task —— View job


Code Review for PR #9534

Tasks:

  • Read repository guidelines (CLAUDE.md)
  • Review interface changes in iSafes.ts
  • Review implementation changes in safes.ts
  • Review test additions in safes.ts test file
  • Provide comprehensive code review feedback

Analysis Complete - Providing Review:

This PR implements filtering of safe key ceremonies based on enabled root slots returned from the WP initialization. Overall, this is a well-implemented feature with good backward compatibility and thorough testing. Here's my detailed review:

Strengths

  1. Excellent Backward Compatibility: The implementation gracefully handles older WP versions by falling back to all 4 slots when enabledRootSlots is absent (enabledRootSlots ?? allSlots)

  2. Type Safety: Good use of TypeScript with proper type definitions and leveraging existing InitializeSafeResponse['enabledRootSlots'] type

  3. Comprehensive Test Coverage: Tests cover all the key scenarios:

    • Decoding enabledRootSlots from server response
    • Subset filtering behavior
    • Threading through generateSafe chain
    • Error handling remains intact
  4. Clean Architecture: The change maintains the existing ceremony structure while adding filtering capability without breaking existing patterns

  5. Good Error Handling: Preserves existing error handling and safe archival on ceremony failures

🔍 Code Quality Observations

Interface Changes (iSafes.ts:28-35)

  • ✅ Good documentation explaining the Flipt-based server decision model
  • ✅ Proper optional field handling with enabledRootSlots?: InitializeSafeResponse['enabledRootSlots']
  • ✅ Updated comments to reflect "enabled slots" vs hardcoded "4 slots"

Implementation (safes.ts:132-149)

  • ✅ Elegant filtering logic using Set for O(1) lookups
  • ✅ Maintains slot ordering (scheme-based) while filtering
  • ✅ Clean ceremony mapping using Record type for better maintainability
  • ✅ Parallel execution preserved with Promise.allSettled(slots.map(...))

Threading Logic (safes.ts:97-105)

  • ✅ Proper data flow from initializeSafecreateSafeKeysfinalizeSafe
  • enabledRootSlots correctly passed through the chain

📝 Minor Suggestions

  1. Type Improvement: Consider extracting the ceremonies record type for reusability:

    type CeremoniesMap = Record<RootKeyType, () => Promise<RootKeyTriplet>>;
  2. Code Comment: The comment on line 137 could be more explicit about the Set optimization:

    // Use Set for O(1) slot filtering - older WP (pre-gating) uses all 4 slots
    const enabled = new Set(enabledRootSlots ?? allSlots);

🧪 Testing Assessment

The test additions are comprehensive and well-structured:

  • Decoding Test (safes.ts:45-60): Verifies proper parsing of enabledRootSlots
  • Filtering Test (safes.ts:185-203): Confirms only enabled ceremonies run and unused coins aren't instantiated
  • Integration Test (safes.ts:277-300): Validates end-to-end threading through generateSafe

Security & Performance

  • No Security Issues: The filtering is based on server-provided data with proper validation
  • Performance Gain: Running fewer ceremonies when slots are filtered reduces overhead
  • Resource Optimization: Unused coin instances aren't created (keychainsByCoin.should.not.have.property('tbtc'))

📋 Final Assessment

This is a solid implementation that adds valuable functionality while maintaining backward compatibility and code quality. The PR achieves its goal of filtering safe key ceremonies based on enabled root slots with minimal risk and good performance characteristics.

Recommendation: ✅ Approve

The code follows BitGoJS patterns, includes comprehensive tests, and handles edge cases appropriately. The feature will allow more efficient safe creation by running only necessary ceremonies.


Review completed for PR #9534 on branch WCN-1658/filter-enabled-root-slots

@danielpeng1
danielpeng1 merged commit 0e69576 into master Aug 21, 2026
38 of 40 checks passed
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