Fix issue where mutex is acquired after state is read in addToken resulting in overwritten state with batched addToken/watchAsset calls
#1768
Conversation
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
…ulting in overwritten state with batched addToken/watchAsset calls
jiexi
approved these changes
Oct 3, 2023
Comment on lines
+1394
to
+1395
| .mockImplementationOnce(() => requestId) | ||
| .mockImplementationOnce(() => '67890'); |
There was a problem hiding this comment.
Suggested change
| .mockImplementationOnce(() => requestId) | |
| .mockImplementationOnce(() => '67890'); | |
| .mockReturnValueOnce(requestId) | |
| .mockReturnValueOnce('67890'); |
Comment on lines
+1400
to
+1401
| state.allTokens?.[ChainId.mainnet]?.[interactingAddress].length === | ||
| 2 |
There was a problem hiding this comment.
Suggested change
| state.allTokens?.[ChainId.mainnet]?.[interactingAddress].length === | |
| 2 | |
| state.tokens.length === 2 |
maybe? but definitely keep the allTokens checks below
| ...anotherAsset, | ||
| }, | ||
| ]); | ||
| generateRandomIdStub.mockRestore(); |
There was a problem hiding this comment.
jest.resetAllMocks() in a beforeEach()?
EDIT: ohh... I see. This pattern is used 4 other times in this file already. Up to you if you want to clean it up or not
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
…esulting in overwritten state with batched addToken/watchAsset calls (#1768) Fixes a subtle bug: when multiple `wallet_watchAsset` calls of ERC20 tokens are batched, only the last of the tokens added was getting added because the calls were not properly single threaded, since the mutex was acquired after reading state in the `addToken` call. ## TokensController ### Fixes - Fixes bug where batched addToken calls overwrite each other because mutex is acquired after reading state
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
…esulting in overwritten state with batched addToken/watchAsset calls (#1768) Fixes a subtle bug: when multiple `wallet_watchAsset` calls of ERC20 tokens are batched, only the last of the tokens added was getting added because the calls were not properly single threaded, since the mutex was acquired after reading state in the `addToken` call. ## TokensController ### Fixes - Fixes bug where batched addToken calls overwrite each other because mutex is acquired after reading state
MajorLift
pushed a commit
that referenced
this pull request
Oct 12, 2023
…esulting in overwritten state with batched addToken/watchAsset calls (#1768) Fixes a subtle bug: when multiple `wallet_watchAsset` calls of ERC20 tokens are batched, only the last of the tokens added was getting added because the calls were not properly single threaded, since the mutex was acquired after reading state in the `addToken` call. ## TokensController ### Fixes - Fixes bug where batched addToken calls overwrite each other because mutex is acquired after reading state
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes a subtle bug: when multiple
wallet_watchAssetcalls of ERC20 tokens are batched, only the last of the tokens added was getting added because the calls were not properly single threaded, since the mutex was acquired after reading state in theaddTokencall.TokensController
Fixes