Skip to content

feat(sdk-api): add registerWithBaseCoin for dynamic token registration#8589

Open
manas-at-bitgo wants to merge 1 commit intomasterfrom
feat/CSHLD-24-register-token
Open

feat(sdk-api): add registerWithBaseCoin for dynamic token registration#8589
manas-at-bitgo wants to merge 1 commit intomasterfrom
feat/CSHLD-24-register-token

Conversation

@manas-at-bitgo
Copy link
Copy Markdown
Contributor

@manas-at-bitgo manas-at-bitgo commented Apr 21, 2026

Summary

  • Add registerWithBaseCoin method to BitGoAPI that wraps GlobalCoinFactory.registerToken() — adds a token to both the global coin map and constructor map in one call
  • Refactor registerWithCoinMap in sdk-coin-eth to be a drop-in replacement for register() — calls register(sdk) internally for hardcoded coins, then registers dynamic ERC20 tokens from a consumer-provided CoinMap
  • Add @bitgo/statics dependency to sdk-api

Motivation

Enables dynamic token onboarding: consumers build a CoinMap from AMS (which drifts ahead of what's baked into @bitgo/statics) and pass it to registerWithCoinMap. New tokens become available via bitgo.coin() without an SDK version bump.

TICKET: CSHLD-24

Test plan

  • register() registers base ETH coins + ERC20/ERC721 constructors
  • registerWithCoinMap() delegates to register() then adds dynamic tokens via registerWithBaseCoin
  • registerWithCoinMap() registers constructors by both type name and contract address
  • Empty ERC20 coin map results in no registerWithBaseCoin calls
  • Unit tests pass: npx mocha --require tsx modules/sdk-coin-eth/test/unit/register.ts

🤖 Generated with Claude Code

@manas-at-bitgo manas-at-bitgo requested review from a team as code owners April 21, 2026 12:45
@linear
Copy link
Copy Markdown

linear Bot commented Apr 21, 2026

@manas-at-bitgo manas-at-bitgo requested a review from Marzooqa April 21, 2026 12:45
@manas-at-bitgo manas-at-bitgo force-pushed the feat/CSHLD-24-register-token branch from 9d9b444 to e6fac36 Compare April 22, 2026 12:27
Copy link
Copy Markdown
Contributor

@pranavjain97 pranavjain97 left a comment

Choose a reason for hiding this comment

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

the approach works but creates duplication that will drift. registerWithCoinMap copies the 4 base coin + ERC721 registrations from register(), and registerWithBaseCoin on BitGoBase is a public interface change for what's really an internal concern. simpler: have registerWithCoinMap call register(sdk) internally, then just add the dynamic token part with GlobalCoinFactory.registerToken() directly. no interface change needed.

also, no tests added for registerWithBaseCoin or the updated registerWithCoinMap.

Comment thread modules/sdk-coin-eth/src/register.ts
Comment thread modules/sdk-coin-eth/src/register.ts Outdated
@manas-at-bitgo manas-at-bitgo force-pushed the feat/CSHLD-24-register-token branch from e6fac36 to 25cc4fe Compare April 23, 2026 10:47
@manas-at-bitgo manas-at-bitgo requested a review from a team as a code owner April 23, 2026 10:47
@manas-at-bitgo manas-at-bitgo force-pushed the feat/CSHLD-24-register-token branch 3 times, most recently from f1e5ea4 to c2a6e6d Compare April 23, 2026 12:18
@manas-at-bitgo
Copy link
Copy Markdown
Contributor Author

@pranavjain97

  • registerWithCoinMap now calls register(sdk) internally — no more duplication
  • Simplified registerWithBaseCoin — dropped the redundant name param since registerToken already registers by baseCoin.name. It's still a public method on BitGoAPI since consumers need it to add tokens to the global coin map at runtime, which is the whole point of dynamic onboarding — can't do that with GlobalCoinFactory directly from outside sdk-api
  • Added unit tests covering register(), registerWithCoinMap(), and the empty coin map case

@manas-at-bitgo manas-at-bitgo force-pushed the feat/CSHLD-24-register-token branch from c2a6e6d to 87e4d9e Compare April 23, 2026 12:22
@manas-at-bitgo manas-at-bitgo force-pushed the feat/CSHLD-24-register-token branch from 87e4d9e to e03258b Compare April 23, 2026 13:19
TICKET: CSHLD-24

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@manas-at-bitgo manas-at-bitgo force-pushed the feat/CSHLD-24-register-token branch from e03258b to dd521b8 Compare April 23, 2026 13:42
veetragjain
veetragjain previously approved these changes Apr 24, 2026
Comment on lines +26 to 31
// Registration for dynamic ERC20 tokens that are not hardcoded in the SDK, but are present in the coin map generated using AMS.
const formattedTokens = getFormattedErc20Tokens(coinMap);
Erc20Token.createTokenConstructors(formattedTokens).forEach(({ name, coinConstructor }) => {
// Register constructor for both type names and contract addresses
sdk.register(name, coinConstructor);
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this not throw an error if coin is not present in the coin map maintained by the coin factory?

setRequestTracer(reqTracer: IRequestTracer): void;
url(path: string, version?: number): string;
register(name: string, coin: CoinConstructor): void;
registerWithBaseCoin(coin: CoinConstructor, baseCoin: Readonly<StaticsBaseCoin>): void;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Base Coin can get confusing, there are two types of base coin, one used for coin map and one used for coin factory, we can rename it to registerWithStaticsCoin or something similar

@veetragjain veetragjain dismissed their stale review April 24, 2026 11:04

Few changes still required

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.

3 participants