Skip to content

feat(statics): add DynamicNetwork and DynamicCoin for AMS-discovered chains#8291

Open
shubham-damkondwar wants to merge 1 commit intomasterfrom
CECHO-439/dynamic-coin-network
Open

feat(statics): add DynamicNetwork and DynamicCoin for AMS-discovered chains#8291
shubham-damkondwar wants to merge 1 commit intomasterfrom
CECHO-439/dynamic-coin-network

Conversation

@shubham-damkondwar
Copy link
Contributor

Ticket: CECHO-439

@shubham-damkondwar shubham-damkondwar force-pushed the CECHO-439/dynamic-coin-network branch from badd853 to 04e55d0 Compare March 24, 2026 11:00
Copy link
Contributor

@manas-at-bitgo manas-at-bitgo left a comment

Choose a reason for hiding this comment

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

Must Fix

1. token.features can be undefined — runtime crash

AmsTokenConfig.features is string[] | undefined. In buildDynamicCoin, token.features as string[] passes undefined through to BaseCoin.validateOptions(), which iterates with for...ofTypeError: options.features is not iterable.

- features: token.features as string[],
+ features: (token.features ?? []) as string[],

2. Missing denom propagation

AmsTokenConfig.denom is never passed to the DynamicCoin constructor. Cosmos-family chains (and others using denoms) will silently lose this field.

  new DynamicCoin({
    ...
    alias: token.alias,
+   denom: token.denom,
  })

Should Fix

3. EVM-centric baseUnit default

baseUnit defaults to 'wei'. For non-EVM chains (UTXO, Cosmos, Substrate), this is incorrect. Consider requiring AMS to always provide baseUnit, or use a family-aware default.

4. No unit tests

Zero test coverage for DynamicCoin, DynamicNetwork, getNetwork(), buildDynamicCoin(). At minimum:

  • DynamicCoin construction with valid and edge-case AMS payloads
  • getNetwork() with JSON string, plain name, and invalid input
  • buildDynamicCoin() with features: undefined (the bug above)

5. getNetwork() resolution order may shadow local statics

JSON parse runs before getNetworkByName(). If AMS sends {"name": "bitcoin", "type": "mainnet", "family": "btc"}, a new DynamicNetwork is created instead of returning the existing Networks.main.bitcoin singleton. Code comparing network references (coin.network === Networks.main.bitcoin) would break.

// Check local statics first, then fall back to JSON parse
const networkObj = getNetworkByName(network);
if (networkObj) return networkObj;
// ... then try JSON parse

6. DynamicNetwork instances aren't cached

Every getNetwork() call with the same JSON string creates a new instance. If multiple AMS-discovered tokens share a network, they'll hold different objects. Consider a Map<string, DynamicNetwork> cache keyed on name+type.

manas-at-bitgo
manas-at-bitgo previously approved these changes Mar 26, 2026
Copy link
Contributor

@manas-at-bitgo manas-at-bitgo left a comment

Choose a reason for hiding this comment

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

lgtm

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