Skip to content

Conversation

@vutuanlinh2k2
Copy link
Contributor

Summary of changes

  • Replace BN, HexString types usage from @polkadot/util to internal and viem
  • Minor code refactors (remove redundant and duplicate code

Proposed area of change

Put an x in the boxes that apply.

  • apps/tangle-dapp
  • apps/tangle-cloud
  • apps/leaderboard
  • libs/tangle-shared-ui
  • libs/ui-components

Associated issue(s)

Specify any issues that can be closed from these changes (e.g. Closes #233).

@vutuanlinh2k2 vutuanlinh2k2 self-assigned this Dec 10, 2025
@vutuanlinh2k2 vutuanlinh2k2 changed the title feat: refactor: reduce code from polkadot lib by replacing BN and HexString import from @polkadot/util to internal and viem Dec 10, 2025
@vutuanlinh2k2 vutuanlinh2k2 marked this pull request as draft December 10, 2025 15:44
- Merged origin/v2 branch containing Tangle v2 EVM migration
- Resolved 80 conflicts (49 modify/delete + 31 content)
- Applied BN refactoring from linh/refactor/update-bn to v2
- Removed all @polkadot/util BN dependencies
- All BN types now use @tangle-network/tangle-shared-ui/bn
- Credits system converted from bigint to BN
- Deprecated Substrate features accept v2 stubs
- Active EVM features use BN for consistency
- Fixed bigint comparisons to use BN.isZero() method
- Updated CreditsButtonProps to use BN instead of bigint
- Removed non-existent utils export from dapp-config
- All credits-related types now consistently use BN
- Fixed unnecessary dependency 'applicationName' in useCallback (WebbProvider)
- Removed useless Fragment in NetworkGuard component
- Created .nxignore to exclude forge lib directories from NX inference
- All TypeScript projects now pass linting without warnings
NetworkType is a string union type ('MAINNET' | 'TESTNET'), not an enum.
Changed default parameter from NetworkType.Mainnet to 'MAINNET' string literal.

Fixes TS2693: 'NetworkType' only refers to a type, but is being used as a value
@vutuanlinh2k2 vutuanlinh2k2 changed the title refactor: reduce code from polkadot lib by replacing BN and HexString import from @polkadot/util to internal and viem refactor: reduce code from polkadot lib by replacing BN and HexString import from @polkadot/util Dec 11, 2025
@vutuanlinh2k2 vutuanlinh2k2 marked this pull request as ready for review December 11, 2025 07:26
@claude
Copy link

claude bot commented Dec 11, 2025

Claude finished @vutuanlinh2k2's task —— View job


PR Review: BN and HexString Refactor

I've completed a comprehensive review of this refactoring PR. Here's my analysis:

Overall Assessment: Good Refactor

This is a well-executed refactoring that reduces dependency on @polkadot/util while maintaining functionality. The approach is sound and follows the codebase patterns.

📋 Code Quality & Best Practices

Strengths:

  • Proper centralization: New BN utilities in libs/tangle-shared-ui/src/bn/ follow the monorepo structure
  • Consistent import patterns: All files now import BN from the internal utility
  • Clean consolidation: Moved useInputAmount from tangle-dapp to shared library (good DRY principle)
  • Type safety maintained: BN types remain consistent throughout

Good utility implementations:

  • bnMax/bnMin functions in utils.ts are well-implemented with proper error handling
  • formatDecimal utility is clean and well-documented with examples
  • Constants file provides commonly used BN values

🔍 Potential Issues Found

⚠️ Incomplete Migration:

Several files still import from @polkadot/util:

  • libs/ui-components/src/utils/convertAddressToBytes32.ts:4 - imports u8aToHex
  • libs/tangle-shared-ui/src/utils/optimizeTxBatch.ts
  • libs/tangle-shared-ui/src/hooks/useOperatorInfo.tsx
  • apps/tangle-dapp/src/utils/getSubstrateLockId.ts

Recommendation: Complete the migration by replacing these remaining imports for full consistency.

⚠️ Missing HexString handling:

The PR mentions HexString refactoring but I don't see a clear replacement strategy. Some files may still need HexString functionality.

🏎️ Performance Considerations

Positive impacts:

  • Reduced bundle size: Fewer polkadot/util imports mean smaller bundles
  • Faster builds: Less dependency on heavy polkadot libraries
  • Better tree-shaking: Internal utilities allow better optimization

⚠️ Potential concerns:

  • BN operations remain the same performance-wise
  • No significant performance regression expected

🛡️ Security Implications

No security concerns:

  • BN.js is still the underlying library, just imported differently
  • Number precision handling remains intact
  • No changes to cryptographic operations

📚 Documentation Updates Needed

Missing documentation:

  • New BN utilities (libs/tangle-shared-ui/src/bn/utils.ts) need JSDoc comments
  • Constants file could benefit from usage examples
  • No CLAUDE.md updates needed as this follows existing patterns

🧪 Testing Considerations

⚠️ Test coverage:

  • Large refactor across 100 files should be thoroughly tested
  • Consider adding unit tests for the new BN utilities
  • Integration tests should verify number handling still works correctly

🔧 Specific Code Comments

libs/tangle-shared-ui/src/bn/utils.ts:3-24

The createCmp function is well-designed but could benefit from JSDoc:

/**
 * Creates a comparison function for BN values
 * @param cmp - Comparison predicate function
 * @returns Function that returns the item matching the comparison
 */
function createCmp<T>(cmp: (a: T, b: T) => boolean): (...items: T[]) => T

apps/tangle-dapp/src/utils/parseChainUnits.ts:25

The TODO comment indicates a known bug that should be addressed:

// TODO: Small bug remains for the special case when the whole part is 0. 
// Example: '0.01' gets parsed as '0.1'!

🎯 Recommendations

  1. Complete the migration by replacing remaining @polkadot/util imports
  2. Add JSDoc comments to new utility functions
  3. Address the parseChainUnits bug mentioned in the TODO
  4. Run comprehensive tests to ensure number handling works correctly
  5. Consider adding unit tests for the new BN utilities

🚀 Final Verdict

✅ APPROVE - This is a solid refactoring that improves the codebase structure. The minor issues mentioned can be addressed in follow-up PRs or quick fixes.

The refactor successfully reduces external dependencies while maintaining functionality and follows good software engineering practices.


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