feat: SOL-46 adds the multichain transactions controller package #5133
Conversation
zone-live
changed the title
There was a problem hiding this comment.
Hello! Thanks for pinging us about this. Just had some comments below. I think reaching 100% test coverage to avoid pain in the future, aligning the controller with our controller guidelines, and adjusting the dependencies in package.json are the most important items here.
|
|
||
| const controllerName = 'MultichainTransactionsController'; | ||
|
|
||
| export type PaginationOptions = { |
There was a problem hiding this comment.
Can we add JSDoc for this type? How is it used?
| /** | ||
| * Default state of the {@link MultichainTransactionsController}. | ||
| */ | ||
| export const defaultState: MultichainTransactionsControllerState = { |
There was a problem hiding this comment.
Can we use a function to construct the default state rather than keep it in a constant? This would align with our controller guidelines: https://github.com/MetaMask/core/blob/main/docs/writing-controllers.md#provide-a-default-representation-of-state
| * using the `persist` flag; and if they can be sent to Sentry or not, using | ||
| * the `anonymous` flag. | ||
| */ | ||
| const MultichainTransactionsControllerMetadata = { |
There was a problem hiding this comment.
The name of the metadata variable should not be capitalized.
| const MultichainTransactionsControllerMetadata = { | |
| const multichainTransactionsControllerMetadata = { |
| state, | ||
| }: { | ||
| messenger: MultichainTransactionsControllerMessenger; | ||
| state: MultichainTransactionsControllerState; |
There was a problem hiding this comment.
The state argument should be a partial version of the eventual state, and should be optional: https://github.com/MetaMask/core/blob/main/docs/writing-controllers.md#accept-an-optional-partial-representation-of-state
| state, | |
| }: { | |
| messenger: MultichainTransactionsControllerMessenger; | |
| state: MultichainTransactionsControllerState; | |
| state = {}, | |
| }: { | |
| messenger: MultichainTransactionsControllerMessenger; | |
| state?: Partial<MultichainTransactionsControllerState>; |
| * Update the transactions of all tracked accounts | ||
| */ | ||
| async updateTransactions() { | ||
| await Promise.allSettled( |
There was a problem hiding this comment.
This is probably the first time I've seen allSettled used in a controller! Nice.
| "@metamask/accounts-controller": "^21.0.0", | ||
| "@metamask/auto-changelog": "^3.4.4", | ||
| "@metamask/keyring-controller": "^19.0.3", | ||
| "@metamask/keyring-internal-api": "^2.0.0", |
There was a problem hiding this comment.
Shouldn't this be in dependencies? I notice that we import this package directly in MultichainTransactionsController.
| "@metamask/keyring-internal-api": "^2.0.0", | ||
| "@metamask/keyring-snap-client": "^2.0.0", | ||
| "@metamask/snaps-controllers": "^9.10.0", | ||
| "@metamask/snaps-sdk": "^6.7.0", |
There was a problem hiding this comment.
Shouldn't this be in dependencies? I notice that we import this package directly in MultichainTransactionsController.
| return; | ||
| } | ||
|
|
||
| this.#handle = setInterval(this.#callback, this.#interval); |
There was a problem hiding this comment.
Is it a concern if the callback takes longer than the interval? If so, what are your thoughts on maintaining the loop using setTimeout instead of setInterval?
| describe('MultichainTransactionsTracker', () => { | ||
| it('starts polling when calling start', async () => { | ||
| const { tracker } = setupTracker(); | ||
| const spyPoller = jest.spyOn(Poller.prototype, 'start'); |
There was a problem hiding this comment.
What are your thoughts on using jest.mock to mock the Poller class? Reaching into the prototype to do so seems a bit odd to me.
| // Every 5s in milliseconds. | ||
| const TRANSACTIONS_TRACKING_INTERVAL = 5 * 1000; | ||
|
|
||
| export class MultichainTransactionsTracker { |
There was a problem hiding this comment.
Thoughts on adding JSDoc for this class? What is the purpose of this class?
Explanation
This PR adds a new package to the Core repo, with the new MultichainTransactions controller, it comes from my original extension PR, and it's responsibility is to track transactions for non-EVM accounts, with different block times, by using the Solana and Bitcoin snaps as the data source. It shares the same approach/structure with the MultichainBalancesController, given that responsibilities are very similar.
References
It relates to the PR opened in extension that will be updated after this one is merged.
Changelog
@metamask/multichain-transactions-controllermultichain-transactions-controllerpackage, with theMultichainTransactionsControllerto track the transactions for non-EVM accounts by using the Solana and Bitcoin snaps as the data source..Checklist