Skip to content
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

Parameterize SmartTransactionsController state by ChainId for MultiChain + Integrate PollingController Mixin #210

Merged
merged 25 commits into from
Oct 19, 2023

Conversation

adonesky1
Copy link
Contributor

@adonesky1 adonesky1 commented Oct 11, 2023

Resolves: https://github.com/MetaMask/MetaMask-planning/issues/1039

Integrates new PollingController abstraction into the SmartTransactionsController, implements the _executePoll method and parameterizes the updateSmartTransactions method by networkClientId such that pending SmartTransaction statuses can be polled for concurrently across different chains.

A note: As is our current controller refactor strategy, these changes are additive, backwards compatible and coexist alongside the existing globally selected network pattern. Once we fully adopt this polling pattern we should no longer access the root liveness and fees state but rather access these values by chainId from livenessByChainId and feesByChainId.

Added

  • Integrates PollingController mixin into the SmartTransactionsController and implements the abstract _executePoll method.

Changed

  • Adds optional options object containing a networkClientId argument to the updateSmartTransaction method which, if passed, is used fetch the chainId that corresponds to the networkClientId and is then used to fetch and update the status of pending smart transactions for that chainId.

@adonesky1 adonesky1 force-pushed the multichain-controller-integration branch from 275b4f2 to d2070ab Compare October 12, 2023 16:10
@adonesky1 adonesky1 force-pushed the multichain-controller-integration branch from b4222f6 to b3a7a22 Compare October 13, 2023 15:47
@adonesky1 adonesky1 force-pushed the multichain-controller-integration branch from b3a7a22 to bcab24b Compare October 13, 2023 18:47
@adonesky1 adonesky1 force-pushed the multichain-controller-integration branch from ebc6818 to c34e0b3 Compare October 13, 2023 21:40
@adonesky1 adonesky1 force-pushed the multichain-controller-integration branch from 8a8b836 to 1a1bcbf Compare October 16, 2023 16:12
@adonesky1 adonesky1 marked this pull request as ready for review October 16, 2023 16:17
@adonesky1 adonesky1 requested a review from a team as a code owner October 16, 2023 16:17
@adonesky1 adonesky1 changed the title Integrating multichain controller upgrades Parameterize SmartTransactionsController state by ChainId for MultiChain + Integrate PollingController Abstraction Oct 16, 2023
@adonesky1 adonesky1 changed the title Parameterize SmartTransactionsController state by ChainId for MultiChain + Integrate PollingController Abstraction Parameterize SmartTransactionsController state by ChainId for MultiChain + Integrate PollingController Mixin Oct 16, 2023
@@ -117,3 +117,5 @@ export type SignedTransaction = any;

// TODO
export type SignedCanceledTransaction = any;

export type Hex = `0x${string}`;

Choose a reason for hiding this comment

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

Could this be imported from @metamask/utils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose we could. I assume this is used somewhere else in the extension dependency graph so probably not adding any weight. I just feel silly adding the full package just to pull in a type here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @mcmire how do you generally think about these cases ☝️ ?

Choose a reason for hiding this comment

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

Drive by comment: 0x{string} isn't Hex, and @metamask/utils is used by lots of other packages, so this shouldn't be in a general utility library if its is to be named Hex. Unbounded (length) hex strings are currently not possible in TypeScript (microsoft/TypeScript#43335).

You can maybe get typescript to full check 30ish characters or so, more if you make use of a "trampoline", but type checking will be so slow it'll be unusable.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true — the Hex type isn't as useful as it could be. For what it's worth we do have some ways to double check that a string is a real hex string at runtime in @metamask/utils, although we might not be using them as much as we should. I tend to think of Hex as more of a signal to readers than an actual constraint for consumers.

Since it's basically an approximation and easy to write, I'm okay with copying this and not importing it from @metamask/utils. If we need something else from utils then we can always bring the package in later.

src/constants.ts Outdated Show resolved Hide resolved
@@ -117,3 +117,5 @@ export type SignedTransaction = any;

// TODO
export type SignedCanceledTransaction = any;

export type Hex = `0x${string}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

That's true — the Hex type isn't as useful as it could be. For what it's worth we do have some ways to double check that a string is a real hex string at runtime in @metamask/utils, although we might not be using them as much as we should. I tend to think of Hex as more of a signal to readers than an actual constraint for consumers.

Since it's basically an approximation and easy to write, I'm okay with copying this and not importing it from @metamask/utils. If we need something else from utils then we can always bring the package in later.

src/SmartTransactionsController.ts Outdated Show resolved Hide resolved
src/SmartTransactionsController.ts Outdated Show resolved Hide resolved
src/SmartTransactionsController.ts Outdated Show resolved Hide resolved
src/SmartTransactionsController.ts Outdated Show resolved Hide resolved
@adonesky1 adonesky1 marked this pull request as draft October 16, 2023 22:25
@socket-security
Copy link

socket-security bot commented Oct 18, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: ws@7.5.9

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

@adonesky1
Copy link
Contributor Author

adonesky1 commented Oct 18, 2023

Time to update jest #210 (comment) @mcmire ? Jest package wasn't changed in this PR so this must be a new audit finding?

@adonesky1
Copy link
Contributor Author

@SocketSecurity ignore ws@7.5.9

jiexi
jiexi previously approved these changes Oct 18, 2023
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Getting closer!

src/SmartTransactionsController.ts Outdated Show resolved Hide resolved
src/SmartTransactionsController.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

One more thing and then I think we're good 😅

src/SmartTransactionsController.ts Outdated Show resolved Hide resolved
src/SmartTransactionsController.test.ts Outdated Show resolved Hide resolved
@adonesky1 adonesky1 requested review from mcmire and jiexi October 19, 2023 14:50
src/SmartTransactionsController.test.ts Outdated Show resolved Hide resolved
src/SmartTransactionsController.test.ts Outdated Show resolved Hide resolved
@mcmire mcmire dismissed their stale review October 19, 2023 20:04

Concerns addressed.

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Thanks for your patience — everything looks good now! (I was getting the test failure locally — I don't think requests are being mocked in this package fully. Should be able to re-run.)

@adonesky1 adonesky1 merged commit a9f5004 into main Oct 19, 2023
16 checks passed
@adonesky1 adonesky1 deleted the multichain-controller-integration branch October 19, 2023 20:36
@legobeat
Copy link
Contributor

legobeat commented Oct 23, 2023

It seems like there are now two different providers that this controller could use. This is currently being hidden by the use of any in this file, but once you lift that you should see. We'll have to account for this, or switch from Ethers to @metamask/eth-query.

@adonesky1 @mcmire I see this went with @metamask/eth-query instead of ethers. It seems to me that etehrs would be preferred. Should we consider migrating before release (#216)?

@adonesky1 adonesky1 mentioned this pull request Oct 23, 2023
adonesky1 added a commit that referenced this pull request Nov 9, 2023
… MultiChain + Integrate PollingController Mixin (#210)"

This reverts commit a9f5004.
adonesky1 added a commit that referenced this pull request Nov 9, 2023
… MultiChain + Integrate PollingController Mixin (#235)

* Revert "Parameterize SmartTransactionsController state by ChainId for MultiChain + Integrate PollingController Mixin (#210)"

This reverts commit a9f5004.

* rebase cleanup
adonesky1 added a commit that referenced this pull request Nov 9, 2023
…ain + Integrate PollingController Mixin (#210)

* integrating multichain controller upgrades

* add test

* cleanup

* need to move confirmSmartTransaction tests into another test since confirmSmartTransaction is now private

* actually for now leave it as a public method

* cleanup

* fix typeerror?

* add test + cleanup

* add a test + bump test coverage

* update type

* fix type issue

* address feedback

* fix provider logic

* update test coverage threshholds

* make a private and public version of updateSmartTransaction

* fix capitalization"

* addressing feedback

* replace @ethersproject/providers with @metamask/eth-query

* cleanup

* address more feedback

* remove @ethersproject/bignumber

* make confirmSmartTransaction a private method

* remove comment

* getTransaction -> getTransactionByHash

* fix ethquery mock
adonesky1 added a commit that referenced this pull request Nov 20, 2023
…ain + Integrate PollingController Mixin (#210)

* integrating multichain controller upgrades

* add test

* cleanup

* need to move confirmSmartTransaction tests into another test since confirmSmartTransaction is now private

* actually for now leave it as a public method

* cleanup

* fix typeerror?

* add test + cleanup

* add a test + bump test coverage

* update type

* fix type issue

* address feedback

* fix provider logic

* update test coverage threshholds

* make a private and public version of updateSmartTransaction

* fix capitalization"

* addressing feedback

* replace @ethersproject/providers with @metamask/eth-query

* cleanup

* address more feedback

* remove @ethersproject/bignumber

* make confirmSmartTransaction a private method

* remove comment

* getTransaction -> getTransactionByHash

* fix ethquery mock
adonesky1 added a commit that referenced this pull request Nov 28, 2023
…ain + Integrate PollingController Mixin (#210)

* integrating multichain controller upgrades

* add test

* cleanup

* need to move confirmSmartTransaction tests into another test since confirmSmartTransaction is now private

* actually for now leave it as a public method

* cleanup

* fix typeerror?

* add test + cleanup

* add a test + bump test coverage

* update type

* fix type issue

* address feedback

* fix provider logic

* update test coverage threshholds

* make a private and public version of updateSmartTransaction

* fix capitalization"

* addressing feedback

* replace @ethersproject/providers with @metamask/eth-query

* cleanup

* address more feedback

* remove @ethersproject/bignumber

* make confirmSmartTransaction a private method

* remove comment

* getTransaction -> getTransactionByHash

* fix ethquery mock
adonesky1 added a commit that referenced this pull request Nov 30, 2023
…ain + Integrate PollingController Mixin (#237)

* Parameterize SmartTransactionsController state by ChainId for MultiChain + Integrate PollingController Mixin (#210)
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.

6 participants