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 a condition in the "wipeSmartTransations" function #323

Merged
merged 4 commits into from
May 2, 2024

Conversation

dan437
Copy link
Collaborator

@dan437 dan437 commented Apr 26, 2024

Description

Fix a condition in the "wipeSmartTransations" function, update tests.

@dan437 dan437 requested a review from a team as a code owner April 26, 2024 11:50
});
});

it('removes transactions from current chainId if ignoreNetwork is false', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps also a case for does not remove transaction from unrelated chainId if ignoreNetwork is true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the test removes transactions from current chainId if ignoreNetwork is false I check if transactions were removed from one chain but not from another one. I've updated a title for that test to make it little clearer.
If ignoreNetwork is true, we want to remove it from all chainIds and for that I already have a test.

@@ -1415,12 +1415,29 @@ describe('SmartTransactionsController', () => {
expect(smartTransactionsController.state).toStrictEqual(prevState);
});

it('removes transactions from current chainId if ignoreNetwork is true', () => {
it('removes transactions from all supported chainIds if ignoreNetwork is true', () => {
Copy link
Contributor

@legobeat legobeat Apr 26, 2024

Choose a reason for hiding this comment

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

Should there also be test(s) for current behavior for unsupported chainIds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've renamed this test to removes transactions from all chains saved in the smartTransactionsState if ignoreNetwork is true to make it clear that we don't care about the supportedChainIds list in this function.

Copy link
Contributor

@legobeat legobeat Apr 29, 2024

Choose a reason for hiding this comment

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

I think I'd still like to see test-coverage for the following cases:

  1. user is on supported network A
  2. user submits smart transaction
  3. user changes network to not supported network B, retaining their existing smart transaction on network A
  4. user calls wipeSmartTransactions
    a. ...with ignoreNetwork: false
    - this will ignore the submitted tx and leave it unmodified. this should be covered.
    b. ...with ignoreNetwork: true
    - seems obvious that this should wipe all txs regardless, but still be nice to test this for the sake of regressions.

infiniteflower
infiniteflower previously approved these changes Apr 26, 2024
@infiniteflower infiniteflower self-requested a review May 1, 2024 14:44
@dan437 dan437 merged commit b848dae into main May 2, 2024
16 checks passed
@dan437 dan437 deleted the update-condition branch May 2, 2024 08:53
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.

3 participants