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: replace nonce 0, flicker, no spending limit + disable duplite rejections #1300

Merged
merged 5 commits into from
Dec 2, 2022

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Dec 1, 2022

What it solves

Resolves isses found here.

How this PR fixes it

  1. Spending limit transactions are not shown creating a transaction via the replacement flow.
  2. The backdrop no longer flickers whent the autocomplete dropdown is open.
  3. Transactions with nonce 0 now open the replacement modal.
  4. Disable duplicate rejection transactions.

How to test it

  1. Attempt to replace a transaction. Observe that spending limit options are not displayed.
  2. Open the nonce autocomplete limit and observe no backdrop flicker.
  3. Attempt to replace a transaction with nonce 0. Clicking "Replace transaction" in the modal should open the next step.
  4. The replacement button is now always shown. Attempting to replace a transaction that already has a rejection transaction will render a disabled rejeection button in the modal.

@iamacook iamacook requested a review from schmanu December 1, 2022 16:51
@iamacook iamacook self-assigned this Dec 1, 2022
@github-actions
Copy link

github-actions bot commented Dec 1, 2022

Branch preview

✅ Deploy successful!

https://tx-replacement-fixes--webcore.review-web-core.5afe.dev

@github-actions
Copy link

github-actions bot commented Dec 1, 2022

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

)}
/>
<>
<Backdrop open={backdrop} />
Copy link
Member

Choose a reason for hiding this comment

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

An interesting workaround, but have you figured out why the flicker happens specifically with this dropdown?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's related to the gas price polling here. The Backdrop seems to be very tempermental between renders.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like the polling re-renders more than necessary in that form. I would take another look at that.

Copy link
Member

Choose a reason for hiding this comment

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

I deleted the backdrop entirely. It now looks the same as the AddressBookInput. Just a dropdown.

Copy link
Member Author

Choose a reason for hiding this comment

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

Without the Backdrop, the options blur into the background though. It was suggested here:

image

cc @usame-algan

@francovenica
Copy link
Contributor

The three issues fixed in this ticket are correct
Spending limit is not available anymore for replacement tx
Tx with nonce 0 can be rejected/replaced
The flicker effect doesn't happen anymore.

Issue: Now that I can replace tx with nonce 0 I noticed this:

In adv options and having only the tx with nonce 0 available for replacement, if you click on the nonce field to see the list of nonces you can replace it shows no list. It would be expected to show the list with only the nonce 0 tx.
0 nonce

How it is expected to behave:
image

Copy link
Member

@schmanu schmanu left a comment

Choose a reason for hiding this comment

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

I'm still puzzled by Backdrop rendering but the workaround works :D

@iamacook iamacook changed the title fix: replace nonce 0, flicker + no spending limit fix: replace nonce 0, flicker, no spending limit + disable duplite rejections Dec 2, 2022
@iamacook
Copy link
Member Author

iamacook commented Dec 2, 2022

Now that I can replace tx with nonce 0 I noticed this:

This should now work:

image

I've also included your findings from the epic here as the PR is still open.

Once you create a rejection and Queue it, the "replace" button is gone, even tho it should be reasonalble to replace even the rejection. (probably the replace button is just following the logic of the old reject button)
What shouldn't be available anymore is the "Reject button" in the "Replace tx modal". The button could be grayed out the tooltip saying "this nonce already has a rejection proposed"

image

@github-actions
Copy link

github-actions bot commented Dec 2, 2022

CLA Assistant Lite All Contributors have signed the CLA.

@katspaugh katspaugh merged commit db45638 into tx-replacement Dec 2, 2022
@katspaugh katspaugh deleted the tx-replacement-fixes branch December 2, 2022 11:19
iamacook added a commit that referenced this pull request Dec 6, 2022
* feat: replace specific transactions (#1053)

* feat: replace specific transactions

* fix: cleanup code

* fix: update title

* style: change `variant`

* fix: replacing `0` nonced transactions

* feat: add autocomplete to `NonceForm` nonce (#1059)

* feat: replacement modal (#1132)

* feat: replace transaction modal w/ stepper

* fix: adjust text + padding/margins

* fix: adjust alignment + add mobile view

* fix: alignment

* fix: tweak autocomplete item text

* fix: shorten text + add `Backdrop`

* Merge branch 'dev' into tx-replacement

* fix: replace nonce 0, flicker, no spending limit + disable duplite rejections (#1300)

* fix: replace nonce 0, flicker + no spending limit

* fix: always allow replacement + nonce `0`

* fix: remove tag

* Rm backdrop

Co-authored-by: katspaugh <[email protected]>

Co-authored-by: katspaugh <[email protected]>
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.

4 participants