-
-
Notifications
You must be signed in to change notification settings - Fork 200
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 MultichainBalancesController
#4965
Open
gantunesr
wants to merge
63
commits into
main
Choose a base branch
from
feat/add-multichain-balances
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,506
−16
Open
Changes from 51 commits
Commits
Show all changes
63 commits
Select commit
Hold shift + click to select a range
950a2eb
chore: add dependencies
gantunesr 12a233a
feat: add MultichainBalancesController, BalancesTracker, and Poller
gantunesr c9b707c
test: add unit test to MultichainBalancesController, BalancesTracker,…
gantunesr b80a2da
Merge branch 'main' of https://github.com/MetaMask/core into feat/add…
gantunesr 0c15e21
fix: dependencies version consistency
gantunesr 27e2d6c
test: update BtcMethod
gantunesr 1890789
test: update BtcMethod
gantunesr c07cadc
chore: add utils and constants
gantunesr 0afee9f
refactor: construction and accounts tracking
gantunesr 9d6d4ec
chore: add comments
gantunesr eca2402
Merge branch 'main' of https://github.com/MetaMask/core into feat/add…
gantunesr 84a029c
chore: small refactor to trackAccount
gantunesr b9192af
test: fix address validation mock
gantunesr 614d072
test: add utils unit test
gantunesr 2d31d3c
test: improve BalancesTracker unit test
gantunesr 425d18a
test: improve Poller unit test
gantunesr c8e3065
test: improve MultichainBalancesController unit test
gantunesr e59a99b
refactor: BalancesTracker constructor
gantunesr 890d317
test: add test to updateBalance
gantunesr 7223308
Merge branch 'main' into feat/add-multichain-balances
gantunesr 21059a9
chore: add logs
gantunesr 8d28a3c
Merge branch 'feat/add-multichain-balances' of https://github.com/Met…
gantunesr 7cc6bc0
Revert "chore: add logs"
gantunesr 4556f0a
chore: update exports
gantunesr d069a64
chore: update exports
gantunesr 9e30e90
chore: update imports
gantunesr e1a582c
chore: add console.error to Poller
gantunesr c3f273d
chore: state update
gantunesr a1e11fb
chore: add logs
gantunesr 3178c25
chore: add more logs
gantunesr 73fa7bf
chore: add more logs
gantunesr bb93981
chore: update logs
gantunesr 00ea290
Merge branch 'main' into feat/add-multichain-balances
gantunesr 243de73
chore: revert changes
gantunesr 7612528
test: fix unit test issues
gantunesr daed2ea
Merge branch 'feat/add-multichain-balances' of https://github.com/Met…
gantunesr 9e67716
fix: lint
gantunesr a49ed0d
chore: BalancesTracker method
gantunesr c548615
chore: update constant
gantunesr 115e329
chore: nit changes
gantunesr 41ccd3b
refactor: updateBalance method
gantunesr 017c657
refactor: BITCOIN_AVG_BLOCK_TIME var name
gantunesr fe5d9ec
chore: add error classes
gantunesr ae590d8
refactor: listAccounts
gantunesr b355d11
refactor: Poller class to use PollerError
gantunesr 9365597
chore: add JSDocs to isBalanceOutdated method
gantunesr 582699f
i tpusjMerge branch 'feat/add-multichain-balances' of https://github.…
gantunesr 84157f0
fix: lint issue with isTracked
gantunesr 2f0bfb8
Merge branch 'main' into feat/add-multichain-balances
gantunesr ef8b0f5
fix: remove unused vars
gantunesr f5d1cea
Merge branch 'main' of https://github.com/MetaMask/core into feat/add…
gantunesr afe5bd2
Merge branch 'main' of https://github.com/MetaMask/core into feat/add…
gantunesr 985aed5
test: fix import for MultichainBalancesController.test
gantunesr 80f4288
chore: fix import in utils file
gantunesr 920699e
Merge branch 'main' into feat/add-multichain-balances
gantunesr bcaee6c
refactor: apply review suggestions
gantunesr 852b238
test: update unit tests
gantunesr d71ad47
test: improve coverage
gantunesr 1807e3c
Merge branch 'main' of https://github.com/MetaMask/core into feat/add…
gantunesr b8a3c6d
Merge branch 'main' of https://github.com/MetaMask/core into feat/add…
gantunesr 9d3797d
Merge branch 'main' of https://github.com/MetaMask/core into feat/add…
gantunesr fca2211
chore: bump @metamask/keyring-api
gantunesr cfe20be
test: update mocks to have account scopes
gantunesr 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
143 changes: 143 additions & 0 deletions
143
packages/assets-controllers/src/MultichainBalancesController/BalancesTracker.test.ts
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 |
---|---|---|
@@ -0,0 +1,143 @@ | ||
import { BtcAccountType, BtcMethod } from '@metamask/keyring-api'; | ||
Check failure on line 1 in packages/assets-controllers/src/MultichainBalancesController/BalancesTracker.test.ts GitHub Actions / Lint, build, and test / Lint (20.x)
|
||
import { KeyringTypes } from '@metamask/keyring-controller'; | ||
import { v4 as uuidv4 } from 'uuid'; | ||
|
||
import { BalancesTracker } from './BalancesTracker'; | ||
import { Poller } from './Poller'; | ||
|
||
const MOCK_TIMESTAMP = 1709983353; | ||
|
||
const mockBtcAccount = { | ||
address: '', | ||
id: uuidv4(), | ||
metadata: { | ||
name: 'Bitcoin Account 1', | ||
importTime: Date.now(), | ||
keyring: { | ||
type: KeyringTypes.snap, | ||
}, | ||
snap: { | ||
id: 'mock-btc-snap', | ||
name: 'mock-btc-snap', | ||
enabled: true, | ||
}, | ||
lastSelected: 0, | ||
}, | ||
options: {}, | ||
methods: [BtcMethod.SendBitcoin], | ||
type: BtcAccountType.P2wpkh, | ||
}; | ||
|
||
/** | ||
* Sets up a BalancesTracker instance for testing. | ||
* @returns The BalancesTracker instance and a mock update balance function. | ||
*/ | ||
function setupTracker() { | ||
const mockUpdateBalance = jest.fn(); | ||
const tracker = new BalancesTracker(mockUpdateBalance); | ||
|
||
return { | ||
tracker, | ||
mockUpdateBalance, | ||
}; | ||
} | ||
|
||
describe('BalancesTracker', () => { | ||
it('starts polling when calling start', async () => { | ||
const { tracker } = setupTracker(); | ||
const spyPoller = jest.spyOn(Poller.prototype, 'start'); | ||
|
||
tracker.start(); | ||
expect(spyPoller).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
it('stops polling when calling stop', async () => { | ||
const { tracker } = setupTracker(); | ||
const spyPoller = jest.spyOn(Poller.prototype, 'stop'); | ||
|
||
tracker.start(); | ||
tracker.stop(); | ||
expect(spyPoller).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
it('is not tracking if none accounts have been registered', async () => { | ||
const { tracker, mockUpdateBalance } = setupTracker(); | ||
|
||
tracker.start(); | ||
await tracker.updateBalances(); | ||
|
||
expect(mockUpdateBalance).not.toHaveBeenCalled(); | ||
}); | ||
|
||
it('tracks account balances', async () => { | ||
const { tracker, mockUpdateBalance } = setupTracker(); | ||
|
||
tracker.start(); | ||
// We must track account IDs explicitly | ||
tracker.track(mockBtcAccount.id, 0); | ||
// Trigger balances refresh (not waiting for the Poller here) | ||
await tracker.updateBalances(); | ||
|
||
expect(mockUpdateBalance).toHaveBeenCalledWith(mockBtcAccount.id); | ||
}); | ||
|
||
it('untracks account balances', async () => { | ||
const { tracker, mockUpdateBalance } = setupTracker(); | ||
|
||
tracker.start(); | ||
tracker.track(mockBtcAccount.id, 0); | ||
await tracker.updateBalances(); | ||
expect(mockUpdateBalance).toHaveBeenCalledWith(mockBtcAccount.id); | ||
|
||
tracker.untrack(mockBtcAccount.id); | ||
await tracker.updateBalances(); | ||
expect(mockUpdateBalance).toHaveBeenCalledTimes(1); // No second call after untracking | ||
}); | ||
|
||
it('tracks account after being registered', async () => { | ||
const { tracker } = setupTracker(); | ||
|
||
tracker.start(); | ||
tracker.track(mockBtcAccount.id, 0); | ||
expect(tracker.isTracked(mockBtcAccount.id)).toBe(true); | ||
}); | ||
|
||
it('does not track account if not registered', async () => { | ||
const { tracker } = setupTracker(); | ||
|
||
tracker.start(); | ||
expect(tracker.isTracked(mockBtcAccount.id)).toBe(false); | ||
}); | ||
|
||
it('does not refresh balance if they are considered up-to-date', async () => { | ||
const { tracker, mockUpdateBalance } = setupTracker(); | ||
|
||
const blockTime = 10 * 60 * 1000; // 10 minutes in milliseconds. | ||
jest | ||
.spyOn(global.Date, 'now') | ||
.mockImplementation(() => new Date(MOCK_TIMESTAMP).getTime()); | ||
|
||
tracker.start(); | ||
tracker.track(mockBtcAccount.id, blockTime); | ||
await tracker.updateBalances(); | ||
expect(mockUpdateBalance).toHaveBeenCalledTimes(1); | ||
|
||
await tracker.updateBalances(); | ||
expect(mockUpdateBalance).toHaveBeenCalledTimes(1); // No second call since the balances is already still up-to-date | ||
|
||
jest | ||
.spyOn(global.Date, 'now') | ||
.mockImplementation(() => new Date(MOCK_TIMESTAMP + blockTime).getTime()); | ||
|
||
await tracker.updateBalances(); | ||
expect(mockUpdateBalance).toHaveBeenCalledTimes(2); // Now the balance will update | ||
}); | ||
|
||
it('throws an error if trying to update balance of an untracked account', async () => { | ||
const { tracker } = setupTracker(); | ||
|
||
await expect(tracker.updateBalance(mockBtcAccount.id)).rejects.toThrow( | ||
`Account is not being tracked: ${mockBtcAccount.id}`, | ||
); | ||
}); | ||
}); |
139 changes: 139 additions & 0 deletions
139
packages/assets-controllers/src/MultichainBalancesController/BalancesTracker.ts
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 |
---|---|---|
@@ -0,0 +1,139 @@ | ||
import { Poller } from './Poller'; | ||
|
||
type BalanceInfo = { | ||
lastUpdated: number; | ||
blockTime: number; | ||
}; | ||
|
||
const BALANCES_TRACKING_INTERVAL = 5000; // Every 5s in milliseconds. | ||
|
||
export class BalancesTracker { | ||
#poller: Poller; | ||
|
||
#updateBalance: (accountId: string) => Promise<void>; | ||
|
||
#balances: Record<string, BalanceInfo> = {}; | ||
|
||
constructor(updateBalanceCallback: (accountId: string) => Promise<void>) { | ||
this.#updateBalance = updateBalanceCallback; | ||
|
||
this.#poller = new Poller( | ||
() => this.updateBalances(), | ||
BALANCES_TRACKING_INTERVAL, | ||
); | ||
} | ||
|
||
/** | ||
* Starts the tracking process. | ||
*/ | ||
start(): void { | ||
this.#poller.start(); | ||
} | ||
|
||
/** | ||
* Stops the tracking process. | ||
*/ | ||
stop(): void { | ||
this.#poller.stop(); | ||
} | ||
|
||
/** | ||
* Checks if an account ID is being tracked. | ||
* | ||
* @param accountId - The account ID. | ||
* @returns True if the account is being tracked, false otherwise. | ||
*/ | ||
isTracked(accountId: string) { | ||
return Object.prototype.hasOwnProperty.call(this.#balances, accountId); | ||
} | ||
|
||
/** | ||
* Asserts that an account ID is being tracked. | ||
* | ||
* @param accountId - The account ID. | ||
* @throws If the account ID is not being tracked. | ||
*/ | ||
assertBeingTracked(accountId: string) { | ||
if (!this.isTracked(accountId)) { | ||
throw new Error(`Account is not being tracked: ${accountId}`); | ||
} | ||
} | ||
|
||
/** | ||
* Starts tracking a new account ID. This method has no effect on already tracked | ||
* accounts. | ||
* | ||
* @param accountId - The account ID. | ||
* @param blockTime - The block time (used when refreshing the account balances). | ||
*/ | ||
track(accountId: string, blockTime: number) { | ||
// Do not overwrite current info if already being tracked! | ||
if (!this.isTracked(accountId)) { | ||
this.#balances[accountId] = { | ||
lastUpdated: 0, | ||
blockTime, | ||
}; | ||
} | ||
} | ||
|
||
/** | ||
* Stops tracking a tracked account ID. | ||
* | ||
* @param accountId - The account ID. | ||
* @throws If the account ID is not being tracked. | ||
*/ | ||
untrack(accountId: string) { | ||
this.assertBeingTracked(accountId); | ||
delete this.#balances[accountId]; | ||
} | ||
|
||
/** | ||
* Update the balances for a tracked account ID. | ||
* | ||
* @param accountId - The account ID. | ||
* @throws If the account ID is not being tracked. | ||
*/ | ||
async updateBalance(accountId: string) { | ||
this.assertBeingTracked(accountId); | ||
|
||
// We check if the balance is outdated (by comparing to the block time associated | ||
// with this kind of account). | ||
// | ||
// This might not be super accurate, but we could probably compute this differently | ||
// and try to sync with the "real block time"! | ||
const info = this.#balances[accountId]; | ||
if (this.#isBalanceOutdated(info)) { | ||
await this.#updateBalance(accountId); | ||
this.#balances[accountId].lastUpdated = Date.now(); | ||
} | ||
} | ||
|
||
/** | ||
* Update the balances of all tracked accounts (only if the balances | ||
* is considered outdated). | ||
*/ | ||
async updateBalances() { | ||
await Promise.allSettled( | ||
Object.keys(this.#balances).map(async (accountId) => { | ||
await this.updateBalance(accountId); | ||
}), | ||
); | ||
} | ||
|
||
/** | ||
* Checks if the balance is outdated according to the provided data. | ||
* | ||
* @param param - The balance info. | ||
* @param param.lastUpdated - The last updated timestamp. | ||
* @param param.blockTime - The block time. | ||
* @returns True if the balance is outdated, false otherwise. | ||
*/ | ||
#isBalanceOutdated({ lastUpdated, blockTime }: BalanceInfo): boolean { | ||
return ( | ||
// Never been updated: | ||
lastUpdated === 0 || | ||
// Outdated: | ||
Date.now() - lastUpdated >= blockTime | ||
); | ||
} | ||
} |
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.
@gantunesr @owencraston we could try/catch here and log and the error here. We could re-use the pattern I suggested with
PollerError
maybe?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.
But wouldn't the Poller class error log already catch any error from this method?
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.
I think no, since we are using a
Promise.allSettled
here, some promises will be marked as "rejected", but no exceptions will be raised. So we would not see any logs IMO.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.
@ccharly is correct, we would need to filter on the promises for fulfilled | rejected to log the state of a promise.