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

feat: replace specific transactions #1053

Merged
merged 5 commits into from
Nov 7, 2022
Merged

feat: replace specific transactions #1053

merged 5 commits into from
Nov 7, 2022

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Nov 3, 2022

What it solves

Resolves #972

How this PR fixes it

Clicking "Replace" now opens the transaction creation modal in "replacement mode". It is possible to create an asset/NFT transfer with the nonce of the chosen transaction, or alternatively a rejection transaction.

How to test it

Queue a transaction then click "Replace". Observe that the transaction-to-be-created will be of the same nonce as that which will be replaced.

Analytics changes

Clicking "Rejection transaction" in the creation modal emits:

  {
    event: "customClick",
    chainId: "",
    eventCategory: "modals",
    eventAction: "Reject transaction"
  }

Screenshots

Note: the button style has changed since recording this GIF.

replacement

New variant:

image

@iamacook iamacook self-assigned this Nov 3, 2022
@github-actions
Copy link

github-actions bot commented Nov 3, 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

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Nov 3, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7aa89fc
Status: ✅  Deploy successful!
Preview URL: https://1a2bfe51.web-core.pages.dev
Branch Preview URL: https://replace-nonce.web-core.pages.dev

View logs

@katspaugh
Copy link
Member

Design-wise, "Rejection transaction" could also be just grey, not red, and say "Empty transaction"? Might be more transparent to the user.
@liliiaorlenko WDYT?

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.

It works very well and I like the implementation.

I have some small suggestions / questions.

@@ -42,7 +44,7 @@ const RejectTxButton = ({
<>
<Track {...TX_LIST_EVENTS.REJECT}>
{compact ? (
<Tooltip title="Reject" arrow placement="top">
<Tooltip title="Replace" arrow placement="top">
<span>
<IconButton onClick={onClick} color="error" size="small" disabled={isDisabled}>
<SvgIcon component={ErrorIcon} inheritViewBox fontSize="small" />
Copy link
Member

Choose a reason for hiding this comment

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

I feel like the ErrorIcon does not capture the replacement feature as much as it did the rejection.

Copy link
Member Author

Choose a reason for hiding this comment

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

@johannesmoormann mentioned that we will work on these designs some more so I think we can revisit this at a later stage.

image

return (
<>
<ModalDialog open dialogTitle="New transaction" onClose={onClose}>
<ModalDialog open dialogTitle={dialogTitle} onClose={onClose}>
<DialogContent>
<Box display="flex" flexDirection="column" alignItems="center" gap={2} pt={7} pb={4} width={240} m="auto">
Copy link
Member

Choose a reason for hiding this comment

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

Should we add some info-text here if the nonce is set explaining how replacement works?

Something along the lines of Replacing a transaction will create a transaction with the same nonce. To actually replace / reject this new transaction has to get executed to use that nonce

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 looked somewhat odd with text inside the modal. I've added a tooltip. What do you think?

tooltip

Copy link
Member

Choose a reason for hiding this comment

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

Nice! I like it.

@iamacook
Copy link
Member Author

iamacook commented Nov 4, 2022

Design-wise, "Rejection transaction" could also be just grey, not red, and say "Empty transaction"? Might be more transparent to the user.

I much prefer the wording. I've adjusted the "variant" to match that of the "Contract interaction" button. I think it looks much better:

image

image

"Contract interaction" for reference:

image

@iamacook iamacook requested a review from schmanu November 4, 2022 10:17
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.

Nice work! 🚀

@iamacook iamacook requested a review from usame-algan November 7, 2022 11:25
@iamacook
Copy link
Member Author

iamacook commented Nov 7, 2022

I will merge this into the epic branch.

@iamacook iamacook merged commit 8f2d1c8 into tx-replacement Nov 7, 2022
@iamacook iamacook deleted the replace-nonce branch November 7, 2022 12:46
Copy link

@elandrea07 elandrea07 left a comment

Choose a reason for hiding this comment

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

approve

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.

5 participants