Transaction replacement - Allow speeding up transactions #3456

hyphenized · 2023-06-08T06:27:48Z

Allows sending a replacement transaction from within an account activity details; A replacement transaction consists of a transaction with the same nonce as another sent before, i.e., the original transaction, usually as an attempt to speed up or cancel it's processing altogether.

Since within activity details we have access to most of the data we need to create this new transaction, we can send this back to the background script through an async thunk, and handle signing and the actual sending from there. Replacement and original transactions are tracked on Redux, a migration was not necessary due to the fact that activities are initialized at extension startup. Furthermore, this data persists until the extension restarts.

This PR also refactors code from activities to accommodate improved typing and internationalization, it also fixes a bug associated to the polling of block prices for custom networks.

Testing Env

SUPPORT_TRANSACTION_REPLACEMENT=true

To Test

Send a transaction
On the activity list, open up the details slide up for the pending transaction that was just made, click on speed up and sign the transaction prompt that follows.
The original transaction should not display in the activity list once the replacement transaction succeeds

Latest build: extension-builds-3456 (as of Fri, 28 Jul 2023 10:37:20 GMT).

kkosiorowska

I haven't tested it yet but the first few comments at first glance.

.env.defaults

background/redux-slices/activities.ts

ui/components/Wallet/WalletActivityDetails.tsx

kkosiorowska

Some visual issues:

Address not displaying correctly

Dropped should be in red colour

I wonder if the last element should have a bottom border. 🤔

ui/_locales/en/messages.json

kkosiorowska · 2023-06-08T13:41:18Z

background/redux-slices/activities.ts

+    const isEIP1559Tx = isEIP1559TransactionRequest(tx)
+
+    if (!tx.gasPrice || (isEIP1559Tx && !tx.maxFeePerGas)) {
+      throw new Error("Cannot speed up transaction without a valid gas price")


Do we not want to display a message in this case? The user does not really know what happens when something goes wrong as in this case.

Screen.Recording.2023-06-08.at.15.30.26.mov

Agree, another concern I have here is related to block times and chains without mempool, for example it doesn't make sense to show the option to speed up transactions on Optimism, and, on Polygon we get fast enough block times that most attempts to speed up result in the replacement transaction being dropped. Maybe we could use block times or check if we have lower than average gas fees to improve UX a bit here 🤔 .

I think we should make an allowlist for the speedup tx button 🤔

jagodarybacka

Couple of comments, haven't tested yet

jagodarybacka · 2023-06-09T11:41:10Z

background/redux-slices/utils/activities-utils.ts

@@ -64,24 +78,32 @@ function getAmount(tx: EnrichedEVMTransaction): string {
  return `${convertToEth(value) || "0"} ${symbol}`
 }

-function getBlockHeight(tx: EnrichedEVMTransaction): string {
+type TransactionStatus = "pending" | "dropped" | "failed" | "completed"


Would it make sense to make it an enum? We are using these strings quite a lot

We've generally avoided using enums in the past as they don't bring much to the table compared to simple type unions like this one (other than extra verbosity 😅). Is there something in particular we want to get out of using them?

Ah I just like their syntax and the fact that I know just looking at it that it is some defined structure, not just a string, I don't have to wonder if this should be for example translated somewhere or not. Not a super strong preference anyway 😉

Ah yeah, fair, this is def a situation where I lean on the editor/language server a lot. I hear your point though, esp the ability to easily identify that something should be translated. Will ponder that a little bit.

Yeah I think these should go into an enum, I ended up reverting this after having conflicting thoughts over similar values for activities. I agree that enums are verbose although one redeeming quality I think would be that with an union we'd have to do an intersection to get one of the members as a fixed type without losing the reference to the group, like the following:

type A = "a" | "b" type B = { // something is "a", if we remove "a" from the type above this becomes never something: A & "a" }

Whereas with enums we can do:

enum A { a, b, } type B = { // If we remove "a" from the enum above we get an error here instead of `never` // as the type for something something: A.a }

One "downside" with these is that if we use string values for this enum then we have to reference the enum members directly, strings/constants do not work.

something is "a", if we remove "a" from the type above this becomes never

This is only an issue if something is never actually interacted with though, right? If things actually interact with it, we should get hit with errors at those points—and if we don't have such uses, then it's probably not a correctness issue… Open to more thinking here, but it feels like an artifact rather than a deep problem.

So I've thought about this a little bit. enums, their mapping of variable names to strings to numbers, the fact that they're a language-level feature (which TypeScript generally doesn't do), and the fact that they have a long list of things to keep in mind in terms of how they work at runtime and compile time, etc, just feel like they're not worth the trouble.

For translations specifically, I think the type union ultimately helps us, because we can use it directly to reference i18n keys and we will know if we've hit all the i18n keys. We can get something similar with enum types if we set them all as explicit strings, but to do that we have to repeat every value (the enum name and then the string value), in addition to referring to the underlying type union in a relatively arcane way.

Overall, enums represent extra layers of indirection between us and the raw values that we're passing around. They give us some small advantages, but I don't think they're worth the tradeoff.

With that said, I'm going to escalate here 😁 and suggest we add a linter rule to prohibit enums altogether! <_<

ui/components/Wallet/WalletActivityDetails.tsx

background/services/chain/index.ts

background/main.ts

jagodarybacka · 2023-06-09T12:17:06Z

background/redux-slices/activities.ts

+
+      const replacement = replacementTransactions.find(
+        (request) =>
+          request.hash === transaction.hash &&


in the other place we are checking tx's author and nonce - so what should we check to know if it is a replacement tx? 🤔

A tx is a replacement if it's a matching nonce, fwiw—but sometimes the previous version of the nonce will get included in the block if it was put into a block by a miner before the replacement made it.

I think this is bad naming on my part, this logic will run a few times, specifically for: pending transactions, completed transactions and once again for these two if they've been enriched. Here we're checking if we're already tracking it to get info about the transaction it's replacing so that we can remove it from the list if the replacement has been mined. I'll add this as a comment.

ui/components/Wallet/WalletActivityDetails.tsx

Shadowfiend · 2023-06-09T13:56:54Z

@hyphenized can you put some more high-level details on approach & changes in the PR description when you get a chance?

jagodarybacka · 2023-06-12T11:55:28Z

I'm not able to scroll the activity slideup
if you browse activities by opening slideup one by one, then fields show previous values for a sec. I think we should use skeleton loaders and clean up the previous values in the slideup.

Screen.Recording.2023-06-12.at.13.46.31.mov

on the list tx is listed as pending while we already know it was dropped

Screen.Recording.2023-06-12.at.13.51.45.mov

When opening and closing two activities in sequence, the first activity details would display momentarily because react tries to reuse the already rendered activity details component

VladUXUI · 2023-06-29T06:44:58Z

Tested this out, and overall looks good! i did however find a couple of things:
1 - When i speed up the transaction the gas setting isn't changed/updated (If custom gas is used for first transaction)
This should have a higher number than the previous tx.
2 - If something was inputed in custom, but custom is NOT the selection in gas, estimated network still says the custom amount.
3 - The transaction that is replaced sometimes gets stuck in "pending" in activity page, but when i click to open it, it says dropped

4 - Can we change dropped to "replaced"? or alternatively don't show the dropped transaction
5 - When speeding up, the initial transaction get's dropped. And i see the message that tx failed to broadcast, this is strange behaviour because users will think it's for the transaction that was sped up.

VladUXUI · 2023-06-29T06:47:16Z

Talked about this with @hyphenized, about gas settings behaviour.

Handles a special case where the replaced transaction is re-added again after being dropped

…dropped.

VladUXUI · 2023-07-26T06:58:37Z

A couple of the points above are still there.

The dropped /replaced transaction is still showing up

The estimated gas is not updated to reflect the gas in Gas settings
There max gas error is no implemented, and the information provided "low" is not enough.

let's not ship this as is and we can come back to it later

VladUXUI · 2023-07-26T07:03:03Z

A bigger issue here is the disconnect between what we show on estimate network and what we show in gas settings.
I think this needs another rethink from me

Shadowfiend · 2023-07-27T01:51:00Z

Got it. Sounds like we need to put this fellow into stasis.

Because this is a bigger piece of work, I'd like to suggest landing the effort so far hidden behind its feature flag. Wdyt @hyphenized ?

hyphenized · 2023-07-27T06:16:19Z

Got it. Sounds like we need to put this fellow into stasis.

Because this is a bigger piece of work, I'd like to suggest landing the effort so far hidden behind its feature flag. Wdyt @hyphenized ?

Easiest might be to block gas settings customization and ship this or, merge this behind the feature flag and break down the problems left into issues. Gas settings set on the replacement transaction are being ignored now and that's causing an immediate drop so will have to take a look into what's happening 😕

Shadowfiend · 2023-07-27T12:26:36Z

I'd like to merge so we can stick to shifting focus this week, but agree on capturing issues.

Let's maybe ask QA to look at a build of this PR with feature flag disabled and confirm transaction flow looks good, spin up issues, and merge it in.

andreachapman · 2023-07-27T13:01:41Z

I'd like to merge so we can stick to shifting focus this week, but agree on capturing issues.

Let's maybe ask QA to look at a build of this PR with feature flag disabled and confirm transaction flow looks good, spin up issues, and merge it in.

I'm happy to do some regression on this with the feature flag disabled. Do we think we'd get it into the coming release cut with the flag disabled? If so, I was just thinking that the regression for that effort would cover it. But if it's going to be separate that fine too, I can test with the disabled flag whenever, if needed.

kkosiorowska

I did some quick tests before merging when feature flag is off. After a swap, the selected activity view shows an error.

Screen.Recording.2023-07-28.at.12.23.00.mov

kkosiorowska · 2023-07-28T10:33:29Z

ui/components/Wallet/WalletActivityDetails.tsx

+    selectTrackedReplacementTransactions
+  )
+
+  const hasReplacementTx = replacementTransactions.some(


Suggested change

const hasReplacementTx = replacementTransactions.some(

const hasReplacementTx = replacementTransactions?.some(

kkosiorowska · 2023-07-28T10:33:40Z

ui/components/Wallet/WalletActivityDetails.tsx

+      tx.chainID === currentAccount.network.chainID
+  )
+
+  const isReplacementTx = replacementTransactions.some(


Suggested change

const isReplacementTx = replacementTransactions.some(

const isReplacementTx = replacementTransactions?.some(

jagodarybacka

Looking at the scope of changes and how much of the most fragile parts of the codebase were changed and refactored I would say we should do full regression and exploratory tests here, on the branch, before merging it even in this state.
I'd say if this has even the slightest chance of messing up with gas prices which can cost our users real money being burned for gas then we shouldn't rush to merge it.

jagodarybacka · 2023-07-28T10:14:30Z

ui/components/Wallet/WalletActivityDetails.tsx

+          <div className="icon_transfer" />
+          <DestinationCard
+            label="To"
+            address={activityItem.recipient.address || "(Contract creation)"}


"(Contract creation)" should be added to translations

jagodarybacka · 2023-07-28T10:17:49Z

ui/components/Wallet/WalletActivityDetails.tsx

@@ -134,24 +186,58 @@ export default function WalletActivityDetails(
      ?.focus()
  }, [activityItem?.hash, blockExplorerUrl])

-  useEffect(() => {
+  useInterval(() => {


shouldn't we clean this interval for activities that are already fully fetched and successful?

jagodarybacka · 2023-07-28T10:41:33Z

background/redux-slices/activities.ts

-    }),
+    ) => {
+      immerState.activities = initializeActivitiesFromTransactions(payload)
+      // Clear these at extension restart


I'm not 100% sure about this, what if we had some transactions and then chrome decides to update the extension? Unlikely but not impossible 🤔

kkosiorowska · 2023-07-28T11:07:24Z

I found one more issue, we aren't able to send transaction for custom networks.

Screen.Recording.2023-07-28.at.12.55.36.mov

Shadowfiend · 2023-07-28T12:47:23Z

Ok, rereading the description and skimming the contents again, it looks like maybe we went too far down a rabbit hole in one PR in that case. I think we should break out the work that got done (refactors, etc) into separate issues referencing this PR, close it, and then we can pick the package up when cycles become available and run the underlying changes through in separate PRs with some git elbow grease.

hyphenized added 5 commits June 7, 2023 16:35

Unsubscribe to network on custom chain removal

b7bc255

Skip managing nonces when there's a replacement transaction

e22833b

Add feature flag

1ab9195

Refactor activity details / poll updates on activity details

3c9aac5

Allow sending replacement transactions from activities

53ae846

hyphenized requested a review from a team June 8, 2023 06:27

kkosiorowska reviewed Jun 8, 2023

View reviewed changes

Fix check for missing gas prices

44c1a54

jagodarybacka reviewed Jun 9, 2023

View reviewed changes

hyphenized added 4 commits June 11, 2023 20:11

Remove duplicate logic

a83cf53

Refactor & rename type for clarity

d33f857

Add comments and fix some i18n strings

4ac4768

Add loading skeleton, missing style

2e3bee7

hyphenized added 4 commits June 16, 2023 14:10

Fix slide up preserving different activity details

1b6d955

When opening and closing two activities in sequence, the first activity details would display momentarily because react tries to reuse the already rendered activity details component

Add scrolll wrap to activity details

ab858d3

Collapse long recipient names

7c1cffa

Improve loading state for pending transactions

f51c723

hyphenized added 7 commits July 4, 2023 22:31

Merge branch 'main' into tx-speed-up

16d4e2c

Optionally pass transaction type

d1dec75

Prevent display of replaced transactions in activities

df746bf

Handles a special case where the replaced transaction is re-added again after being dropped

Restrict the snackbar from displaying when replaced transactions are …

96adea1

…dropped.

Show replacement fees in signing network settings

e44bc6a

Avoid releasing nonces for replaced and dropped transactions

f73a481

Merge branch 'main' into tx-speed-up

0efeb71

Merge branch 'main' into tx-speed-up

792ab9d

Merge branch 'main' into tx-speed-up

fcdd36d

kkosiorowska suggested changes Jul 28, 2023

View reviewed changes

jagodarybacka reviewed Jul 28, 2023

View reviewed changes

hyphenized closed this Jul 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transaction replacement - Allow speeding up transactions #3456

Transaction replacement - Allow speeding up transactions #3456

hyphenized commented Jun 8, 2023 •

edited by github-actions bot

Loading

kkosiorowska left a comment

kkosiorowska left a comment

kkosiorowska Jun 8, 2023

hyphenized Jun 12, 2023

jagodarybacka Jun 12, 2023

jagodarybacka left a comment

jagodarybacka Jun 9, 2023

Shadowfiend Jun 9, 2023 •

edited

Loading

jagodarybacka Jun 9, 2023

Shadowfiend Jun 9, 2023

hyphenized Jun 12, 2023

Shadowfiend Jun 16, 2023

jagodarybacka Jun 9, 2023

Shadowfiend Jun 9, 2023

hyphenized Jun 12, 2023

Shadowfiend commented Jun 9, 2023

jagodarybacka commented Jun 12, 2023 •

edited

Loading

VladUXUI commented Jun 29, 2023

VladUXUI commented Jun 29, 2023 •

edited

Loading

VladUXUI commented Jul 26, 2023

VladUXUI commented Jul 26, 2023

Shadowfiend commented Jul 27, 2023

hyphenized commented Jul 27, 2023

Shadowfiend commented Jul 27, 2023

andreachapman commented Jul 27, 2023

kkosiorowska left a comment

kkosiorowska Jul 28, 2023

kkosiorowska Jul 28, 2023

jagodarybacka left a comment

jagodarybacka Jul 28, 2023

jagodarybacka Jul 28, 2023

jagodarybacka Jul 28, 2023

kkosiorowska commented Jul 28, 2023

Shadowfiend commented Jul 28, 2023

	const hasReplacementTx = replacementTransactions.some(
	const hasReplacementTx = replacementTransactions?.some(

	const isReplacementTx = replacementTransactions.some(
	const isReplacementTx = replacementTransactions?.some(

Transaction replacement - Allow speeding up transactions #3456

Transaction replacement - Allow speeding up transactions #3456

Conversation

hyphenized commented Jun 8, 2023 • edited by github-actions bot Loading

Testing Env

To Test

kkosiorowska left a comment

Choose a reason for hiding this comment

kkosiorowska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagodarybacka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend commented Jun 9, 2023

jagodarybacka commented Jun 12, 2023 • edited Loading

VladUXUI commented Jun 29, 2023

VladUXUI commented Jun 29, 2023 • edited Loading

VladUXUI commented Jul 26, 2023

VladUXUI commented Jul 26, 2023

Shadowfiend commented Jul 27, 2023

hyphenized commented Jul 27, 2023

Shadowfiend commented Jul 27, 2023

andreachapman commented Jul 27, 2023

kkosiorowska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagodarybacka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkosiorowska commented Jul 28, 2023

Shadowfiend commented Jul 28, 2023

hyphenized commented Jun 8, 2023 •

edited by github-actions bot

Loading

Shadowfiend Jun 9, 2023 •

edited

Loading

jagodarybacka commented Jun 12, 2023 •

edited

Loading

VladUXUI commented Jun 29, 2023 •

edited

Loading