-
Notifications
You must be signed in to change notification settings - Fork 302
feat(sdk-api): add registerWithBaseCoin for dynamic token registration #8589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,16 @@ export const register = (sdk: BitGoBase): void => { | |
| }; | ||
|
|
||
| export const registerWithCoinMap = (sdk: BitGoBase, coinMap: CoinMap): void => { | ||
| Erc20Token.createTokenConstructors(getFormattedErc20Tokens(coinMap)).forEach(({ name, coinConstructor }) => { | ||
| register(sdk); | ||
|
|
||
| // 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| sdk.register(name, coinConstructor); | ||
| }); | ||
|
Comment on lines
+26
to
31
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. registering a token that already exists in |
||
| // Add new tokens to the global coin map so they're available for lookup | ||
| formattedTokens.forEach((token) => { | ||
| sdk.registerWithBaseCoin(Erc20Token.createTokenConstructor(token), coinMap.get(token.type)); | ||
| }); | ||
| }; | ||
|
manas-at-bitgo marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| import sinon from 'sinon'; | ||
| import assert from 'assert'; | ||
| import { BitGoAPI } from '@bitgo/sdk-api'; | ||
| import { coins, Erc20Coin } from '@bitgo/statics'; | ||
| import { register, registerWithCoinMap } from '../../src/register'; | ||
| import { Erc20Token } from '../../src/erc20Token'; | ||
| import { Erc721Token } from '../../src/erc721Token'; | ||
|
|
||
| describe('ETH Register', function () { | ||
| let bitgo: BitGoAPI; | ||
| let registerSpy: sinon.SinonSpy; | ||
| let registerWithBaseCoinSpy: sinon.SinonSpy; | ||
|
|
||
| beforeEach(function () { | ||
| bitgo = new BitGoAPI({ env: 'test' }); | ||
| registerSpy = sinon.spy(bitgo, 'register'); | ||
| registerWithBaseCoinSpy = sinon.spy(bitgo, 'registerWithBaseCoin'); | ||
| }); | ||
|
|
||
| afterEach(function () { | ||
| registerSpy.restore(); | ||
| registerWithBaseCoinSpy.restore(); | ||
| }); | ||
|
|
||
| describe('register', function () { | ||
| it('should register base coins and token constructors', function () { | ||
| register(bitgo); | ||
|
|
||
| const registeredNames = registerSpy.getCalls().map((call) => call.args[0]); | ||
|
|
||
| // Base coins should be registered | ||
| assert.ok(registeredNames.includes('eth')); | ||
| assert.ok(registeredNames.includes('gteth')); | ||
| assert.ok(registeredNames.includes('teth')); | ||
| assert.ok(registeredNames.includes('hteth')); | ||
|
|
||
| // ERC20 and ERC721 tokens should be registered | ||
| const erc20Count = Erc20Token.createTokenConstructors().length; | ||
| const erc721Count = Erc721Token.createTokenConstructors().length; | ||
| assert.strictEqual(registerSpy.callCount, 4 + erc20Count + erc721Count); | ||
| }); | ||
| }); | ||
|
|
||
| describe('registerWithCoinMap', function () { | ||
| it('should call register internally for base coins and tokens', function () { | ||
| registerWithCoinMap(bitgo, coins); | ||
|
|
||
| const registeredNames = registerSpy.getCalls().map((call) => call.args[0]); | ||
|
|
||
| // Base coins should be registered via register() | ||
| assert.ok(registeredNames.includes('eth')); | ||
| assert.ok(registeredNames.includes('gteth')); | ||
| assert.ok(registeredNames.includes('teth')); | ||
| assert.ok(registeredNames.includes('hteth')); | ||
| }); | ||
|
|
||
| it('should register dynamic ERC20 tokens via registerWithBaseCoin', function () { | ||
| registerWithCoinMap(bitgo, coins); | ||
|
|
||
| // registerWithBaseCoin should have been called for dynamic tokens | ||
| assert.ok(registerWithBaseCoinSpy.callCount > 0); | ||
|
|
||
| // Each call should pass a valid baseCoin from the coinMap | ||
| for (let i = 0; i < registerWithBaseCoinSpy.callCount; i++) { | ||
| const call = registerWithBaseCoinSpy.getCall(i); | ||
| const baseCoin = call.args[1]; | ||
| assert.ok(coins.has(baseCoin.name), `${baseCoin.name} should exist in the coin map`); | ||
| } | ||
| }); | ||
|
|
||
| it('should not call registerWithBaseCoin when coin map has no ERC20 tokens', function () { | ||
| // Create a coin map with only base coins (no ERC20 tokens) | ||
| const limitedCoinMap = coins.filter((coin) => !(coin instanceof Erc20Coin)); | ||
|
|
||
| registerWithCoinMap(bitgo, limitedCoinMap); | ||
|
|
||
| // registerWithBaseCoin should not be called since no ERC20 tokens are in the map | ||
| assert.strictEqual(registerWithBaseCoinSpy.callCount, 0); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import { | |
| GetSharingKeyOptions, | ||
| IRequestTracer, | ||
| } from '../api'; | ||
| import { BaseCoin as StaticsBaseCoin } from '@bitgo/statics'; | ||
| import { IBaseCoin } from './baseCoin'; | ||
| import { CoinConstructor } from './coinFactory'; | ||
| import { EnvironmentName } from './environments'; | ||
|
|
@@ -35,4 +36,5 @@ export interface BitGoBase { | |
| setRequestTracer(reqTracer: IRequestTracer): void; | ||
| url(path: string, version?: number): string; | ||
| register(name: string, coin: CoinConstructor): void; | ||
| registerWithBaseCoin(coin: CoinConstructor, baseCoin: Readonly<StaticsBaseCoin>): void; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to veetragjain: |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the only caller is
registerWithCoinMap, does this need to be onBitGoBase? could callGlobalCoinFactory.registerTokendirectly