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

Prevent bug where incorrect symbol used for token rates #1496

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Jul 13, 2023

Explanation

There is a bug present in the TokenRatesController where a network switch can result in token rates being requested for the wrong network.

This can happen when the network state update event comes before currency rate controller state update event. The controller only works correctly if the currency rate controller state update event happens first.

Instead of getting the ticker from the currency rate controller, we now get it directly from the network controller. This simplifies the controller and ensures that we always use the currency symbol corresponding with the current chain ID.

References

This is tangentially related to #1466

Changelog

@metamask/assets-controllers

  • BREAKING: Remove onCurrencyRateStateChange constructor parameter from TokenRatesController
  • Fixed: Fix bug where token rates would be invalid if event handlers were triggered in the wrong order

Checklist

  • I've updated the test suite for new or updated code as appropriate
    • The condition that would fail is no longer possible, so there is no way to add a test case for it. The tests for this controller are very difficult to work with at the moment anyway, which is partly what motivated this change
  • 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

Base automatically changed from make-ticker-mandatory to main July 13, 2023 22:10
There is a bug present in the TokenRatesController where a network
switch can result in token rates being requested for the wrong network.

This can happen when the network state update event comes before
currency rate controller state update event. The controller only works
correctly if the currency rate controller state update event happens
first.

Instead of getting the ticker from the currency rate controller, we now
get it directly from the network controller. This simplifies the
controller and ensures that we always use the currency symbol
corresponding with the current chain ID.
@Gudahtt Gudahtt force-pushed the fix-token-rate-event-ordering-bug branch from 9a4e42e to cef335c Compare July 13, 2023 22:25
@Gudahtt Gudahtt marked this pull request as ready for review July 13, 2023 22:26
@Gudahtt Gudahtt requested a review from a team as a code owner July 13, 2023 22:26
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.

Ah, I understand now. This seems a lot better.

@Gudahtt Gudahtt merged commit e40b313 into main Jul 13, 2023
91 checks passed
@Gudahtt Gudahtt deleted the fix-token-rate-event-ordering-bug branch July 13, 2023 22:46
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
There is a bug present in the TokenRatesController where a network
switch can result in token rates being requested for the wrong network.

This can happen when the network state update event comes before
currency rate controller state update event. The controller only works
correctly if the currency rate controller state update event happens
first.

Instead of getting the ticker from the currency rate controller, we now
get it directly from the network controller. This simplifies the
controller and ensures that we always use the currency symbol
corresponding with the current chain ID.
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
There is a bug present in the TokenRatesController where a network
switch can result in token rates being requested for the wrong network.

This can happen when the network state update event comes before
currency rate controller state update event. The controller only works
correctly if the currency rate controller state update event happens
first.

Instead of getting the ticker from the currency rate controller, we now
get it directly from the network controller. This simplifies the
controller and ensures that we always use the currency symbol
corresponding with the current chain ID.
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.

2 participants