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

fix(transaction-controller): Return global ethQuery when !isMultichainEnabled #4390

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented Jun 7, 2024

Explanation

There is a test stating that "MultichainTrackingHelper.getEthQuery should always return ethQuery for global provider when !isMultichainEnabled`".

It was not, but the test was passing anyway due to the mock being hardcoded to do so. This fixes that.

Also refactors tests to actually test for it (separately in #4391).


(Aside: I'm not sure why there even is a distinction between "global provider" and "chain-specific providers" in the first place - seems like there should never be a reason for both to exist simultaneously?) Answer: This is transitory and the "global provider" us due to be refactored away.

References

Related

Blocking

Changelog

@metamask/transaction-controller

  • FIXED: MultichainTrackingHelper.getEthQuery now returns global ethQuery with

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@legobeat legobeat changed the title fix(transaction-controller): fix(transaction-controller): Return global ethQuery when !isMultichainEnabled Jun 7, 2024
@legobeat legobeat marked this pull request as ready for review June 7, 2024 00:52
@legobeat legobeat requested a review from a team as a code owner June 7, 2024 00:52
dbrans
dbrans previously approved these changes Jun 7, 2024
@legobeat legobeat force-pushed the fix-multichaintrackinghelper-use-global-when-no-multi branch from 5a05293 to 31b2ea0 Compare June 7, 2024 01:46
@legobeat legobeat requested review from a team June 7, 2024 02:35
mcmire
mcmire previously approved these changes Jun 7, 2024
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.

Makes sense.

@legobeat legobeat force-pushed the fix-multichaintrackinghelper-use-global-when-no-multi branch from 31b2ea0 to a365586 Compare June 8, 2024 00:48
@legobeat
Copy link
Contributor Author

legobeat commented Jun 8, 2024

(Conflict resolution)

@legobeat legobeat requested review from a team June 8, 2024 00:49
@legobeat legobeat force-pushed the fix-multichaintrackinghelper-use-global-when-no-multi branch from a365586 to 5a311c5 Compare June 10, 2024 00:23
@legobeat legobeat requested review from mcmire and dbrans June 10, 2024 00:33
@dbrans
Copy link
Contributor

dbrans commented Jun 10, 2024

Also refactors tests to actually test for it (separately in #4391).

Hi @legobeat, I couldn’t find the test for !this.#isMultichainEnabled in #4391. It’s usually clearer to include tests in the same PR, especially for future reference.

@legobeat
Copy link
Contributor Author

legobeat commented Jun 10, 2024

Also refactors tests to actually test for it (separately in #4391).

Hi @legobeat, I couldn’t find the test for !this.#isMultichainEnabled in #4391. It’s usually clearer to include tests in the same PR, especially for future reference.

The existing test is here.

See also the multichainTrackerHelperMock, which always uses and returns the same provider unconditionally for getEthQuery and getProvider.

@legobeat
Copy link
Contributor Author

legobeat commented Jun 10, 2024

The test-related changes have now been separated into #4391. This PR now contains the fix in isolation.

…eturn ethQuery for global provider when !isMultichainEnabled
@legobeat legobeat force-pushed the fix-multichaintrackinghelper-use-global-when-no-multi branch from 3f23743 to bb80e40 Compare June 10, 2024 17:26
@legobeat legobeat merged commit 316035e into MetaMask:main Jun 10, 2024
113 checks passed
legobeat added a commit that referenced this pull request Jun 11, 2024
legobeat added a commit that referenced this pull request Jun 11, 2024
legobeat added a commit that referenced this pull request Jun 11, 2024
legobeat added a commit that referenced this pull request Jun 11, 2024
legobeat added a commit that referenced this pull request Jun 12, 2024
legobeat added a commit to legobeat/metamask-controllers that referenced this pull request Jun 12, 2024
legobeat added a commit that referenced this pull request Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants