Adapt TxParametersDetail to display a future safeNonce #2982

DiogoSoaress · 2021-11-10T11:39:10Z

What it solves

Resolves #2904

How this PR fixes it

When the user edits the transaction params and sets a safeNonce greater than the current nonce

How to test it

On creating transaction "Review" step, edit the safeNonce to be greater than the current one.
Then:

the gas estimate area is replaced with error copy
the advanced menu has been expanded
the offending nonce is highlighted red.
updated warning copy

Screenshots

github-actions · 2021-11-10T11:39:25Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2021-11-10T11:43:21Z

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	2	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions · 2021-11-10T11:46:09Z

Safe Rinkeby
Safe Mainnet
Safe Polygon
Safe BSC
Safe Arbitrum
Unified app on dev: https://safe-team.dev.gnosisdev.com/app/

coveralls · 2021-11-10T11:46:27Z

Pull Request Test Coverage Report for Build 1462197080

15 of 17 (88.24%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 31.68%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/routes/safe/components/Transactions/helpers/TxParametersDetail/index.tsx	9	11	81.82%

Totals
Change from base Build 1453880886:	0.1%
Covered Lines:	2985
Relevant Lines:	8383

💛 - Coveralls

github-actions · 2021-11-10T11:51:42Z

E2E Tests Passed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1462223974

… review modals

iamacook

Looking good. I have a few questions and found a few places you can improve

iamacook · 2021-11-12T13:51:36Z

src/components/ReviewInfoText/index.tsx

+  isCreation,
+  isExecution,
+  isOffChainSignature,
+  safeNonce,


Assign a default value here and then you won't need to use a fallback on line 26.

iamacook · 2021-11-12T13:52:19Z

src/components/ReviewInfoText/index.tsx

+            {transactionsToGo}
+          </Text>
+          {` transaction${
+            transactionsToGo > 1 ? 's' : ''


Nice perfectionism 😉

iamacook · 2021-11-12T13:53:10Z

src/components/TransactionsFees/index.tsx

@@ -4,7 +4,7 @@ import { getNetworkInfo } from 'src/config'
 import { TransactionFailText } from 'src/components/TransactionFailText'
 import { Text } from '@gnosis.pm/safe-react-components'

-type TransactionFailTextProps = {
+export type TransactionFailTextProps = {


I'm not sure how we approach exporting types. I would use my Parameters<typeof TransactionFees>[0] trick instead.

Yes, you are right. Works like a charm 👌

iamacook · 2021-11-12T13:55:12Z

src/routes/safe/components/Balances/SendModal/screens/ContractInteraction/Review/index.tsx

-              txEstimationExecutionStatus={txEstimationExecutionStatus}
-            />
-          </div>
+          <ReviewInfoText


Why did you remove the class here?

iamacook · 2021-11-12T13:55:39Z

...utes/safe/components/Balances/SendModal/screens/ContractInteraction/ReviewCustomTx/index.tsx

-                txEstimationExecutionStatus={txEstimationExecutionStatus}
-              />
-            </Block>
+            <ReviewInfoText


Why did you remove the and class here?

iamacook · 2021-11-12T13:58:56Z

src/routes/safe/components/Transactions/helpers/TxParametersDetail/index.tsx

  const threshold = useSelector(currentSafeThreshold) || 1
  const defaultParameterStatus = isOffChainSignature && threshold > 1 ? 'ETH_HIDDEN' : 'ENABLED'

+  const { safeNonce } = txParameters


Assign a default here and you will not have to use a fallback?

iamacook · 2021-11-12T13:59:06Z

src/routes/safe/components/Transactions/helpers/TxParametersDetail/index.tsx

  const threshold = useSelector(currentSafeThreshold) || 1
  const defaultParameterStatus = isOffChainSignature && threshold > 1 ? 'ETH_HIDDEN' : 'ENABLED'

+  const { safeNonce } = txParameters
+  const isSafeNonceFuture = parseInt(safeNonce || '', 10) > nonce
+  const [isAccordionExpanded, setIsAccordionExpanded] = useState(isSafeNonceFuture)


You should strictly type this.

iamacook · 2021-11-12T13:59:33Z

src/routes/safe/components/Transactions/helpers/TxParametersDetail/index.tsx

  if (!isTransactionExecution && !isTransactionCreation && isOffChainSignature) {
    return null
  }

+  const onChangeExpand = () => {
+    setIsAccordionExpanded(!isAccordionExpanded)


Here it should be fine, but you should normally use the previous state value by means of a callback.

iamacook · 2021-11-12T14:00:14Z

src/components/ReviewInfoText/index.tsx

+  const transactionsToGo = parseInt(safeNonce || '', 10) - nonce
+
+  return (
+    <ReviewInfoTextWrapper data-testid={testId}>


You're missing the styling that seems to have been applied everywhere else here.

If we are talking about the

padding: ${sm} ${lg}

That's why I've passed that styling to the to the ReviewInfoTextWrapper styled component.

Thats why I also removed some of the wrapping elements/classes across the Modals as they were repeating this same css property but dispersed across the codebase.

No, I mean in reference to all of my other comments regarding classes.

iamacook · 2021-11-12T14:00:52Z

src/routes/safe/components/Settings/Advanced/RemoveGuardModal.tsx

@@ -154,12 +153,13 @@ export const RemoveGuardModal = ({ onClose, guardAddress }: RemoveGuardModalProp
                  isOffChainSignature={isOffChainSignature}
                />
              </Block>
-              <Row className={cn(classes.modalDescription, classes.gasCostsContainer)}>
-                <TransactionFees
+              <Row className={classes.modalDescription}>


Why did you remove the <Row> and class here?

DiogoSoaress · 2021-11-12T15:39:24Z

I check the and components before removing them. They basically wrap the children prop inside a div. I also verified that I was not getting rid of any particular styling before removing the gasCostsContainer class in the elements.
From my testing the modals are rendered like before but please let me know if it was unnecessary.
The aim was to unify the same wrapping style for the <ReviewInfoText /> inside that component.

iamacook · 2021-11-12T15:42:38Z

I check the and components before removing them. They basically wrap the children prop inside a div. I also verified that I was not getting rid of any particular styling before removing the gasCostsContainer class in the elements. From my testing the modals are rendered like before but please let me know if it was unnecessary. The aim was to unify the same wrapping style for the <ReviewInfoText /> inside that component.

Some were <div>s but there were also <Block>s and <Rows>s. Were all of the gasCostsContainers the same stylistically?

DiogoSoaress · 2021-11-12T15:46:19Z

Yes, all the gasCostsContainers were applying the same styles: background and padding. All referring to the same Modal section.
I think it is way more manageable now and this section should have a unified look

francovenica · 2021-11-15T03:27:20Z

Works fine, a small detail:
The snapshot in the original issue 2904 has also the text in red

…saction

DiogoSoaress · 2021-11-15T08:39:01Z

@francovenica I've fixed it! I also made sure that the Advanced Options section is extended when we enter the Review step with a suggested safe nonce in the future (before it was only opening after you edit).

francovenica · 2021-11-15T12:10:01Z

Looks good to me.
I like the idea of displaying the advanced options if they have to change the nonce.

Diogo Soares added 4 commits November 11, 2021 19:18

Adapt TxParametersDetail to display a future safeNonce

6e1f4c6

Create ReviewInfoText component to display the extra text info in the…

4930fb7

… review modals

Replace <TransactionFees /> call in the Modals by <ReviewInfoText />

786c457

Removed obsolete gasCostsContainer css style

d8cedac

DiogoSoaress force-pushed the safenonce-out-of-order-warning branch from 37a9847 to d8cedac Compare November 11, 2021 18:18

DiogoSoaress marked this pull request as ready for review November 11, 2021 18:19

DiogoSoaress marked this pull request as draft November 11, 2021 18:26

Pass safeNonce to the <ReviewInfoText />

50d90da

DiogoSoaress marked this pull request as ready for review November 11, 2021 18:45

DiogoSoaress marked this pull request as draft November 12, 2021 06:42

Diogo Soares added 2 commits November 12, 2021 12:21

Fix mock in tests

117bee4

Add tests for ReviewInfoText component

58f3bae

DiogoSoaress marked this pull request as ready for review November 12, 2021 13:47

DiogoSoaress requested a review from iamacook November 12, 2021 13:47

iamacook reviewed Nov 12, 2021

View reviewed changes

Diogo Soares added 2 commits November 12, 2021 16:30

Implement default values instead of fallbacks

6d5aef6

Use the Parameters<T> util instead of exporting the component type props

5b38ed6

DiogoSoaress requested a review from iamacook November 12, 2021 15:42

iamacook approved these changes Nov 12, 2021

View reviewed changes

Diogo Soares added 2 commits November 15, 2021 09:32

Make "Safe nonce" red if we are reviewing a future transaction

820c768

Open the Advanced Options Accordion if we are reviewing a future tran…

d5548a6

…saction

Merge branch 'dev' into safenonce-out-of-order-warning

18f7227

DiogoSoaress merged commit 434fa9f into dev Nov 15, 2021

DiogoSoaress deleted the safenonce-out-of-order-warning branch November 15, 2021 12:58

github-actions bot locked and limited conversation to collaborators Nov 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt TxParametersDetail to display a future safeNonce #2982

Adapt TxParametersDetail to display a future safeNonce #2982

DiogoSoaress commented Nov 10, 2021 •

edited

Loading

github-actions bot commented Nov 10, 2021

github-actions bot commented Nov 10, 2021 •

edited

Loading

github-actions bot commented Nov 10, 2021

coveralls commented Nov 10, 2021 •

edited

Loading

github-actions bot commented Nov 10, 2021 •

edited

Loading

iamacook left a comment

iamacook Nov 12, 2021

iamacook Nov 12, 2021

iamacook Nov 12, 2021

DiogoSoaress Nov 12, 2021

iamacook Nov 12, 2021

iamacook Nov 12, 2021

iamacook Nov 12, 2021

iamacook Nov 12, 2021

iamacook Nov 12, 2021

iamacook Nov 12, 2021

DiogoSoaress Nov 12, 2021

iamacook Nov 12, 2021

iamacook Nov 12, 2021

DiogoSoaress commented Nov 12, 2021

iamacook commented Nov 12, 2021

DiogoSoaress commented Nov 12, 2021

francovenica commented Nov 15, 2021

DiogoSoaress commented Nov 15, 2021

francovenica commented Nov 15, 2021

Adapt TxParametersDetail to display a future safeNonce #2982

Adapt TxParametersDetail to display a future safeNonce #2982

Conversation

DiogoSoaress commented Nov 10, 2021 • edited Loading

What it solves

How this PR fixes it

How to test it

Screenshots

github-actions bot commented Nov 10, 2021

github-actions bot commented Nov 10, 2021 • edited Loading

ESLint Summary View Full Report

github-actions bot commented Nov 10, 2021

coveralls commented Nov 10, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1462197080

💛 - Coveralls

github-actions bot commented Nov 10, 2021 • edited Loading

iamacook 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiogoSoaress commented Nov 12, 2021

iamacook commented Nov 12, 2021

DiogoSoaress commented Nov 12, 2021

francovenica commented Nov 15, 2021

DiogoSoaress commented Nov 15, 2021

francovenica commented Nov 15, 2021

DiogoSoaress commented Nov 10, 2021 •

edited

Loading

github-actions bot commented Nov 10, 2021 •

edited

Loading

coveralls commented Nov 10, 2021 •

edited

Loading

github-actions bot commented Nov 10, 2021 •

edited

Loading