-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
feat: add and use Accounts API for Account balance calls #4781
Merged
Prithpal-Sooriya
merged 20 commits into
main
from
NOTIFY-1179/add-account-api-to-token-detection-controller
Oct 18, 2024
Merged
Changes from 10 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
7cee69b
feat: add multichain accounts API services
Prithpal-Sooriya 6e4171a
docs: scaffold and comment sections of TokenDetectionController
Prithpal-Sooriya b52827e
Merge branch 'main' of github.com:MetaMask/core into NOTIFY-1179/add-…
Prithpal-Sooriya d201250
feat: add accounts API support networks endpoint
Prithpal-Sooriya 816ca78
refactor: rename fetchSupportedNetworks function
Prithpal-Sooriya d4c37ef
feat: add token detection via API method and connect to detection con…
Prithpal-Sooriya 9f5724d
refactor: add barrel file
Prithpal-Sooriya 9adebed
test: add test coverage for tokenDetectionController
Prithpal-Sooriya 4e08cba
Merge branch 'main' of github.com:MetaMask/core into NOTIFY-1179/add-…
Prithpal-Sooriya 0911070
docs: update test util jsdoc comment
Prithpal-Sooriya 8664211
docs: remove todo/note comments
Prithpal-Sooriya 228515c
test: add ignore comments for untestable statements
Prithpal-Sooriya 200beb7
Merge branch 'main' of github.com:MetaMask/core into NOTIFY-1179/add-…
Prithpal-Sooriya 4a464ab
Merge branch 'main' into NOTIFY-1179/add-account-api-to-token-detecti…
Prithpal-Sooriya 87c4155
Merge branch 'main' of github.com:MetaMask/core into NOTIFY-1179/add-…
Prithpal-Sooriya cc53352
refactor: use existing enums and constants
Prithpal-Sooriya 7157e10
Merge branch 'NOTIFY-1179/add-account-api-to-token-detection-controll…
Prithpal-Sooriya 9bd0f70
Merge branch 'main' into NOTIFY-1179/add-account-api-to-token-detecti…
Prithpal-Sooriya 56e242c
Merge branch 'main' into NOTIFY-1179/add-account-api-to-token-detecti…
Prithpal-Sooriya 909fde8
Merge branch 'main' into NOTIFY-1179/add-account-api-to-token-detecti…
Prithpal-Sooriya File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,12 @@ import { | |
buildInfuraNetworkConfiguration, | ||
} from '../../network-controller/tests/helpers'; | ||
import { formatAggregatorNames } from './assetsUtil'; | ||
import * as MutliChainAccountsServiceModule from './multi-chain-accounts-service'; | ||
import { | ||
MOCK_GET_BALANCES_RESPONSE, | ||
createMockGetBalancesResponse, | ||
} from './multi-chain-accounts-service/mocks/mock-get-balances'; | ||
import { MOCK_GET_SUPPORTED_NETWORKS_RESPONSE } from './multi-chain-accounts-service/mocks/mock-get-supported-networks'; | ||
import { TOKEN_END_POINT_API } from './token-service'; | ||
import type { | ||
AllowedActions, | ||
|
@@ -46,9 +52,11 @@ import { | |
} from './TokenDetectionController'; | ||
import { | ||
getDefaultTokenListState, | ||
type TokenListMap, | ||
type TokenListState, | ||
type TokenListToken, | ||
} from './TokenListController'; | ||
import type { Token } from './TokenRatesController'; | ||
import type { | ||
TokensController, | ||
TokensControllerState, | ||
|
@@ -173,9 +181,25 @@ function buildTokenDetectionControllerMessenger( | |
}); | ||
} | ||
|
||
const mockMultiChainAccountsService = () => { | ||
const mockFetchSupportedNetworks = jest | ||
.spyOn(MutliChainAccountsServiceModule, 'fetchSupportedNetworks') | ||
.mockResolvedValue(MOCK_GET_SUPPORTED_NETWORKS_RESPONSE.fullSupport); | ||
const mockFetchMultiChainBalances = jest | ||
.spyOn(MutliChainAccountsServiceModule, 'fetchMultiChainBalances') | ||
.mockResolvedValue(MOCK_GET_BALANCES_RESPONSE); | ||
|
||
return { | ||
mockFetchSupportedNetworks, | ||
mockFetchMultiChainBalances, | ||
}; | ||
}; | ||
|
||
describe('TokenDetectionController', () => { | ||
const defaultSelectedAccount = createMockInternalAccount(); | ||
|
||
mockMultiChainAccountsService(); | ||
|
||
beforeEach(async () => { | ||
nock(TOKEN_END_POINT_API) | ||
.get(getTokensPath(ChainId.mainnet)) | ||
|
@@ -2236,6 +2260,236 @@ describe('TokenDetectionController', () => { | |
}, | ||
); | ||
}); | ||
|
||
/** | ||
* Test Utility - Arrange and Act `detectTokens()` with the Accounts API feature | ||
* RPC flow will return `sampleTokenA` and the Accounts API flow will use `sampleTokenB` | ||
* @param props - options to modify these tests | ||
* @param props.overrideMockTokensCache - change the tokens cache | ||
* @param props.mockMultiChainAPI - change the Accounts API responses | ||
* @param props.overrideMockTokenGetState - change the external TokensController state | ||
* @returns properties that can be used for assertions | ||
*/ | ||
const arrangeActTestDetectTokensWithAccountsAPI = async (props?: { | ||
/** Overwrite the tokens cache inside Tokens Controller */ | ||
overrideMockTokensCache?: (typeof sampleTokenA)[]; | ||
mockMultiChainAPI?: ReturnType<typeof mockMultiChainAccountsService>; | ||
overrideMockTokenGetState?: Partial<TokensControllerState>; | ||
}) => { | ||
const { | ||
overrideMockTokensCache = [sampleTokenA, sampleTokenB], | ||
mockMultiChainAPI, | ||
overrideMockTokenGetState, | ||
} = props ?? {}; | ||
|
||
// Arrange - RPC Tokens Flow - Uses sampleTokenA | ||
const mockGetBalancesInSingleCall = jest.fn().mockResolvedValue({ | ||
[sampleTokenA.address]: new BN(1), | ||
}); | ||
|
||
// Arrange - API Tokens Flow - Uses sampleTokenB | ||
const { mockFetchSupportedNetworks, mockFetchMultiChainBalances } = | ||
mockMultiChainAPI ?? mockMultiChainAccountsService(); | ||
|
||
if (!mockMultiChainAPI) { | ||
mockFetchSupportedNetworks.mockResolvedValue([1]); | ||
mockFetchMultiChainBalances.mockResolvedValue( | ||
createMockGetBalancesResponse([sampleTokenB.address], 1), | ||
); | ||
} | ||
|
||
// Arrange - Selected Account | ||
const selectedAccount = createMockInternalAccount({ | ||
address: '0x0000000000000000000000000000000000000001', | ||
}); | ||
|
||
// Arrange / Act - withController setup + invoke detectTokens | ||
const { callAction } = await withController( | ||
{ | ||
options: { | ||
disabled: false, | ||
getBalancesInSingleCall: mockGetBalancesInSingleCall, | ||
useAccountsAPI: true, // USING ACCOUNTS API | ||
}, | ||
mocks: { | ||
getSelectedAccount: selectedAccount, | ||
getAccount: selectedAccount, | ||
}, | ||
}, | ||
async ({ | ||
controller, | ||
mockTokenListGetState, | ||
callActionSpy, | ||
mockTokensGetState, | ||
}) => { | ||
const tokenCacheData: TokenListMap = {}; | ||
overrideMockTokensCache.forEach( | ||
(t) => | ||
(tokenCacheData[t.address] = { | ||
name: t.name, | ||
symbol: t.symbol, | ||
decimals: t.decimals, | ||
address: t.address, | ||
occurrences: 1, | ||
aggregators: t.aggregators, | ||
iconUrl: t.image, | ||
}), | ||
); | ||
|
||
mockTokenListGetState({ | ||
...getDefaultTokenListState(), | ||
tokensChainsCache: { | ||
'0x1': { | ||
timestamp: 0, | ||
data: tokenCacheData, | ||
}, | ||
}, | ||
}); | ||
|
||
if (overrideMockTokenGetState) { | ||
mockTokensGetState({ | ||
...getDefaultTokensState(), | ||
...overrideMockTokenGetState, | ||
}); | ||
} | ||
|
||
// Act | ||
await controller.detectTokens({ | ||
networkClientId: NetworkType.mainnet, | ||
selectedAddress: selectedAccount.address, | ||
}); | ||
|
||
return { | ||
callAction: callActionSpy, | ||
}; | ||
}, | ||
); | ||
|
||
const assertAddedTokens = (token: Token) => | ||
expect(callAction).toHaveBeenCalledWith( | ||
'TokensController:addDetectedTokens', | ||
[token], | ||
{ | ||
chainId: ChainId.mainnet, | ||
selectedAddress: selectedAccount.address, | ||
}, | ||
); | ||
|
||
const assertTokensNeverAdded = () => | ||
expect(callAction).not.toHaveBeenCalledWith( | ||
'TokensController:addDetectedTokens', | ||
); | ||
|
||
return { | ||
assertAddedTokens, | ||
assertTokensNeverAdded, | ||
mockFetchMultiChainBalances, | ||
mockGetBalancesInSingleCall, | ||
rpcToken: sampleTokenA, | ||
apiToken: sampleTokenB, | ||
}; | ||
}; | ||
|
||
it('should trigger and use Accounts API for detection', async () => { | ||
const { | ||
assertAddedTokens, | ||
mockFetchMultiChainBalances, | ||
apiToken, | ||
mockGetBalancesInSingleCall, | ||
} = await arrangeActTestDetectTokensWithAccountsAPI(); | ||
|
||
expect(mockFetchMultiChainBalances).toHaveBeenCalled(); | ||
expect(mockGetBalancesInSingleCall).not.toHaveBeenCalled(); | ||
assertAddedTokens(apiToken); | ||
}); | ||
|
||
/** | ||
* TODO - discuss if this is correct. | ||
* | ||
* If the Accounts API succeeds, but the tokens cannot be added (unable to create `Token` shape) | ||
* Then should we add no tokens & then finish? | ||
* | ||
* If we want to, we could then do a pass with the RPC flow? But would it be necessary? | ||
* - DEV - we can just add a simple check at the end of the API flow where if no tokens were added, then count it as a failure and perform the RPC flow? | ||
*/ | ||
it('uses the Accounts API but does not add unknown tokens', async () => { | ||
// API returns sampleTokenB | ||
// As this is not a known token (in cache), then is not added | ||
const { | ||
assertTokensNeverAdded, | ||
mockFetchMultiChainBalances, | ||
mockGetBalancesInSingleCall, | ||
} = await arrangeActTestDetectTokensWithAccountsAPI({ | ||
overrideMockTokensCache: [sampleTokenA], | ||
}); | ||
|
||
expect(mockFetchMultiChainBalances).toHaveBeenCalled(); | ||
expect(mockGetBalancesInSingleCall).not.toHaveBeenCalled(); | ||
assertTokensNeverAdded(); | ||
}); | ||
|
||
it('fallbacks from using the Accounts API if fails', async () => { | ||
// Test 1 - fetch supported networks fails | ||
let mockAPI = mockMultiChainAccountsService(); | ||
mockAPI.mockFetchSupportedNetworks.mockRejectedValue( | ||
new Error('Mock Error'), | ||
); | ||
let actResult = await arrangeActTestDetectTokensWithAccountsAPI({ | ||
mockMultiChainAPI: mockAPI, | ||
}); | ||
|
||
expect(actResult.mockFetchMultiChainBalances).not.toHaveBeenCalled(); // never called as could not fetch supported networks... | ||
expect(actResult.mockGetBalancesInSingleCall).toHaveBeenCalled(); // ...so then RPC flow was initiated | ||
actResult.assertAddedTokens(actResult.rpcToken); | ||
|
||
// Test 2 - fetch multi chain fails | ||
mockAPI = mockMultiChainAccountsService(); | ||
mockAPI.mockFetchMultiChainBalances.mockRejectedValue( | ||
new Error('Mock Error'), | ||
); | ||
actResult = await arrangeActTestDetectTokensWithAccountsAPI({ | ||
mockMultiChainAPI: mockAPI, | ||
}); | ||
|
||
expect(actResult.mockFetchMultiChainBalances).toHaveBeenCalled(); // API was called, but failed... | ||
expect(actResult.mockGetBalancesInSingleCall).toHaveBeenCalled(); // ...so then RPC flow was initiated | ||
actResult.assertAddedTokens(actResult.rpcToken); | ||
}); | ||
|
||
/** | ||
* TODO - discuss if this is correct. | ||
* | ||
* If the Accounts API succeeds, but the tokens cannot be added (token already added) | ||
* Then should we add no tokens (if they are all added) & then finish? | ||
* | ||
* If we want to, we could then do a pass with the RPC flow? But would it be necessary? | ||
* - DEV - we can just add a simple check at the end of the API flow where if no tokens were added, then count it as a failure and perform the RPC flow? | ||
*/ | ||
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. Yep lets discuss. This flow happens when we receive tokens from our API, but these tokens are already added into the wallet (as detected tokens, ignored tokens, etc). So if the token is already added, we will skip it. |
||
it('uses the Accounts API but does not add tokens that are already added', async () => { | ||
// Here we populate the token state with a token that exists in the tokenAPI. | ||
// So the token retrieved from the API should not be added | ||
const { assertTokensNeverAdded, mockFetchMultiChainBalances } = | ||
await arrangeActTestDetectTokensWithAccountsAPI({ | ||
overrideMockTokenGetState: { | ||
allDetectedTokens: { | ||
'0x1': { | ||
'0x0000000000000000000000000000000000000001': [ | ||
{ | ||
address: sampleTokenB.address, | ||
name: sampleTokenB.name, | ||
symbol: sampleTokenB.symbol, | ||
decimals: sampleTokenB.decimals, | ||
aggregators: sampleTokenB.aggregators, | ||
}, | ||
], | ||
}, | ||
}, | ||
}, | ||
}); | ||
|
||
expect(mockFetchMultiChainBalances).toHaveBeenCalled(); | ||
assertTokensNeverAdded(); | ||
}); | ||
}); | ||
}); | ||
|
||
|
@@ -2415,6 +2669,7 @@ async function withController<ReturnValue>( | |
getBalancesInSingleCall: jest.fn(), | ||
trackMetaMetricsEvent: jest.fn(), | ||
messenger: buildTokenDetectionControllerMessenger(controllerMessenger), | ||
useAccountsAPI: false, | ||
...options, | ||
}); | ||
try { | ||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Yeah lets discuss this scenario.
This is because we are using the
TokenCache
to create the correct token shape.The
Token
shape requires theaggregators
andimage
fields which the API does not provide us.If the new token from the API does not exist in this cache, then we need to skip it.