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.

Sign up for GitHub

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

Jump to bottom

feat(suite): disable tx bumpfee except lowestnonc #14395

Merged
merged 1 commit into from
Sep 19, 2024
Merged

feat(suite): disable tx bumpfee except lowestnonc #14395

merged 1 commit into from
Sep 19, 2024

Conversation

enjojoy
Copy link
Contributor

@enjojoy enjojoy commented Sep 17, 2024 •
edited
Loading

Description

Disable all the bump fees buttons except the transaction with the lowest nonce (the oldest transaction). Tooltip is displayed only on the disabled buttons.

Related Issue

Resolve #13529

Screenshots:

image

@enjojoy enjojoy force-pushed the feat/disable-bump-fee branch 3 times, most recently from 8d173d4 to 90a6b17 Compare September 18, 2024 09:52
@enjojoy enjojoy marked this pull request as ready for review September 18, 2024 09:52
@enjojoy enjojoy marked this pull request as draft September 18, 2024 09:53
@enjojoy enjojoy self-assigned this Sep 18, 2024
@enjojoy enjojoy marked this pull request as ready for review September 18, 2024 11:23
@enjojoy
Copy link
Contributor Author

enjojoy commented Sep 18, 2024

Learn more link - https://trezor.io/learn/a/replace-by-fee-rbf-ethereum

@enjojoy enjojoy force-pushed the feat/disable-bump-fee branch from 90a6b17 to 28531cb Compare September 18, 2024 11:31
@enjojoy enjojoy force-pushed the feat/disable-bump-fee branch from 28531cb to 8154de5 Compare September 18, 2024 11:32
@MiroslavProchazka
Copy link
Contributor

What about using "oldest pending transaction", instead of first? Does that make sense?

Lemonexe
Lemonexe reviewed Sep 18, 2024
View reviewed changes
Copy link
Contributor

@Lemonexe Lemonexe left a comment

Choose a reason for hiding this comment

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

Codewise looks good, just some comments to make it neat.
And please, create the unit test if possible.

I didn't yet get to test it, then I'll finish the CR..

packages/suite/src/components/wallet/TransactionItem/TransactionItem.tsx Outdated Show resolved Hide resolved
suite-common/wallet-utils/src/transactionUtils.ts Outdated Show resolved Hide resolved
suite-common/wallet-utils/src/transactionUtils.ts Outdated Show resolved Hide resolved
Lemonexe
Lemonexe requested changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@Lemonexe Lemonexe left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I tested it and it works correctly for ethereum-type networks ✔️
But it also disables button & displays tooltip for non-ethereum network txs (bitcoin testnet) ❌
I believe this is undesired, right?

@enjojoy
Copy link
Contributor Author

enjojoy commented Sep 18, 2024

I tested it and it works correctly for ethereum-type networks ✔️ But it also disables button & displays tooltip for non-ethereum network txs (bitcoin testnet) ❌ I believe this is undesired, right?

Yes, thank you, will fix that

@tomasklim
Copy link
Member

What about using "oldest pending transaction", instead of first? Does that make sense?

Oldest by nonce, not by time

@enjojoy enjojoy force-pushed the feat/disable-bump-fee branch 5 times, most recently from 28d9b21 to 8d57659 Compare September 19, 2024 11:31
@enjojoy enjojoy requested a review from Lemonexe September 19, 2024 11:31
@enjojoy enjojoy force-pushed the feat/disable-bump-fee branch from 8d57659 to cbcc6dc Compare September 19, 2024 11:33
Lemonexe
Lemonexe approved these changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@Lemonexe Lemonexe left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, it's working as expected ✔️
Codewise looking great 🚀

getTransactionWithLowestNonce could do with unit tests

@enjojoy enjojoy force-pushed the feat/disable-bump-fee branch from cbcc6dc to c5d0a5a Compare September 19, 2024 12:17
@enjojoy
Copy link
Contributor Author

enjojoy commented Sep 19, 2024

Thanks, it's working as expected ✔️ Codewise looking great 🚀

getTransactionWithLowestNonce could do with unit tests, but oh well, I'm guilty of untested code too 😅 Can be done later 🤷

I pushed the tests 😼

@enjojoy enjojoy force-pushed the feat/disable-bump-fee branch from c5d0a5a to 3b8e312 Compare September 19, 2024 12:26
Lemonexe
Lemonexe approved these changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@Lemonexe Lemonexe left a comment

Choose a reason for hiding this comment

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

Good job adding the unit tests, now the code is a bit safer 👍

@enjojoy enjojoy merged commit 5b6ea2a into develop Sep 19, 2024
14 of 24 checks passed
@enjojoy enjojoy deleted the feat/disable-bump-fee branch September 19, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lemonexe Lemonexe Lemonexe approved these changes

@tomasklim tomasklim Awaiting requested review from tomasklim

@MiroslavProchazka MiroslavProchazka Awaiting requested review from MiroslavProchazka MiroslavProchazka is a code owner

@marekrjpolak marekrjpolak Awaiting requested review from marekrjpolak marekrjpolak is a code owner

Assignees

@enjojoy enjojoy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disable “Bump Fee” for txs with higher nonce than the lowest pending
4 participants
@enjojoy @MiroslavProchazka @tomasklim @Lemonexe