Skip to content

feat(sdk-coin-sol): add SPL getTokenType and SolCoin programId guardrails#8904

Merged
abhijeet8986 merged 1 commit into
masterfrom
CHALO-506-sol-single-asset-consolidation
Jun 2, 2026
Merged

feat(sdk-coin-sol): add SPL getTokenType and SolCoin programId guardrails#8904
abhijeet8986 merged 1 commit into
masterfrom
CHALO-506-sol-single-asset-consolidation

Conversation

@abhijeet8986
Copy link
Copy Markdown
Contributor

@abhijeet8986 abhijeet8986 commented Jun 1, 2026

Ticket: CHALO-506

Description

M1 BitGoJS prerequisites for SOL single-asset consolidation (Phase 1). Wallet-platform will gate behavior behind wallet-platform_sol-consolidation_single-asset-enable; this PR does not change consolidation runtime behavior in WP.

Summary

  • Add getTokenType(): 'SPL' on @bitgo/sdk-coin-sol SolToken for SDK parity with TRX/ERC-20 token coins (used by WP for consolidateToken intent routing when WP adds the same method on its SolToken).
  • Add a statics regression test ensuring every SolCoin has a valid programId (classic SPL or Token-2022). WP resolves mint/programId at build time via coins.get(tokenName) — not via formatted SolTokenConfig.

Type of change

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

How Has This Been Tested?

  • Unit tests added in modules/sdk-coin-sol/test/unit/solToken.ts:
    getTokenType() returns 'SPL' for mainnet (sol:spx) and testnet (tsol:usdc) via existing TestBitGo / bitgo.coin() setup

  • Unit test added in modules/statics/test/unit/coins.ts:
    All SolCoin entries have programId ∈ { TokenProgramId, Token2022ProgramId }

@abhijeet8986 abhijeet8986 requested a review from a team as a code owner June 1, 2026 08:25
@linear-code
Copy link
Copy Markdown

linear-code Bot commented Jun 1, 2026

CHALO-506

@abhijeet8986
Copy link
Copy Markdown
Contributor Author

@claude please review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 2, 2026

Claude finished @abhijeet8986's task —— View job


Code Review for SPL Token Support

Tasks:

  • Read and analyze changed files
  • Review implementation quality and consistency
  • Check test coverage and approach
  • Provide comprehensive feedback

Overall Assessment: LGTM with Minor Suggestions

This PR successfully implements the required prerequisites for SOL single-asset consolidation. The changes are minimal, focused, and well-tested.

📋 Summary of Changes

  1. Added getTokenType(): 'SPL' method to SolToken class for wallet-platform consolidation routing
  2. Added comprehensive unit tests for both mainnet and testnet tokens
  3. Added programId validation test to ensure all SOL tokens have valid program IDs (classic SPL or Token-2022)

🔍 Detailed Review

Implementation Quality: ✅ Excellent

modules/sdk-coin-sol/src/solToken.ts:

  • Method signature: Clean and consistent with the pattern (returns string)
  • Documentation: Well-documented JSDoc explaining purpose for wallet-platform
  • Implementation: Simple and correct - returns hardcoded 'SPL' as expected
  • Positioning: Logical placement in the class between getBaseFactor() and existing boolean flags

Test Coverage: ✅ Comprehensive

modules/sdk-coin-sol/test/unit/solToken.ts:

  • Mainnet test: Uses existing sol:spx token via the established test setup
  • Testnet test: Tests tsol:usdc token with proper type casting
  • Integration: Leverages existing TestBitGo infrastructure correctly
  • Coverage: Both test cases cover the core functionality adequately

modules/statics/test/unit/coins.ts:

  • Validation scope: Properly filters SOL family coins and SolCoin instances
  • Test logic: Uses should.containEql() to validate programId is in allowed set
  • Safety: Guards against invalid coin types with proper filtering
  • Imports: Clean import of ProgramID enum

Code Consistency: ✅ Good

  • Follows established patterns in the SolToken class
  • JSDoc style matches other methods in the file
  • Method placement is logical within the class structure
  • Return type is consistent with similar token type methods

🎯 Specific Observations

✅ Strengths

  1. Minimal scope: Changes are focused and don't introduce unnecessary complexity
  2. Future-proof: The programId validation test ensures all SOL coins maintain valid program IDs
  3. No breaking changes: Purely additive functionality
  4. Well-documented: Clear purpose explained in JSDoc
  5. Proper testing: Tests both networks and uses established testing patterns

💡 Minor Suggestions

  1. Consider adding return type annotation for extra clarity:

    getTokenType(): string {
      return 'SPL';
    }

    (Though this is already implied and not strictly necessary)

  2. Test description could be more specific:

    it('should return SPL token type for mainnet token (sol:spx)', function () {
    it('should return SPL token type for testnet token (tsol:usdc)', function () {

🔄 Comparison with Similar Implementations

After reviewing the codebase, I found that:

  • TRX tokens don't currently have a getTokenType() method
  • ERC-20 tokens use different patterns (like transferBuilder.getTokenType())
  • This implementation follows a simpler, more direct pattern which is appropriate for the SOL ecosystem

Conclusion

This PR successfully implements the M1 prerequisites for SOL single-asset consolidation. The code is:

  • Clean and well-structured
  • Properly tested
  • Consistent with existing patterns
  • Ready for integration

The implementation correctly provides the getTokenType(): 'SPL' method that wallet-platform will use for consolidation intent routing, and the regression test ensures programId integrity across all SOL coins.

Recommendation: ✅ APPROVE


🔗 Job run: https://github.com/BitGo/BitGoJS/actions/runs/XXXXXX
🌿 Branch: CHALO-506-sol-single-asset-consolidation

Copy link
Copy Markdown
Contributor

@bhuvanr159 bhuvanr159 left a comment

Choose a reason for hiding this comment

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

LGTM

@abhijeet8986 abhijeet8986 merged commit 4e99bf1 into master Jun 2, 2026
22 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