(Fix) - #1561 Outgoing instead of custom tx for sending collectibles by Agupane · Pull Request #1567 · 5afe/safe-react

Agupane · 2020-11-03T15:28:30Z

Closes #1561

Update: This will also fix #1325

Description

The main point of the fix is on the isSendERC721Transaction function, which checks if there is a tokenID on the tx params
I read the code and found that the function getTxData wasn't easy to read to I improved it a little

Note about #1325:

I refactored the logic of isSendERC721Transaction because the old way in which it was checking if a tx is or not an ERC721 transaction wasn't working well and wasn't very accurate. I think later when we use the all transaction API this function won't be needed anymore. But right now wasn't working well. I started debugging issue Incorrect amount for ERC20 token transfer #1325 and found that this function was returning true for the transaction and it's an ERC20 tx not an ERC721. That's why the frontend was always displaying 1 as the number of transferred tokens

It looks like this now:

github-actions · 2020-11-03T15:28:47Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2020-11-03T15:32:58Z

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

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

ghost · 2020-11-03T15:45:10Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2020-11-03T17:49:46Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/volta/app

…to feature/#1561-outgoing-as-custom-tx

ghost · 2020-11-04T13:51:15Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/volta/app

ghost · 2020-11-04T13:53:46Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/rinkeby/app

fernandomg

Beautiful refactor on getTxData. Can we add tests to it? To be sure that nothing went wrong.

Also left a couple comments.

src/routes/safe/store/models/types/transactions.d.ts

fernandomg · 2020-11-04T16:07:43Z

src/logic/tokens/utils/tokenHelpers.ts

-    (isTokenTransfer(tx) && !knownTokens?.get(tx.to))
+    (txCode?.includes(SAFE_TRANSFER_FROM_WITHOUT_DATA_HASH) && !sameAddress(tx.to, ENS_TOKEN_CONTRACT)) ||
+    (isTokenTransfer(tx) && !knownTokens?.get(tx.to)) ||
+    hasERC721Transfer


I'm not sure how reliable this isSendERC721Transaction function is.

Is it safe to just rely on what the tx-service returns? If so, maybe we can just verify that hasERC721Transfer @dasanra, any opinion?

I refactored this could you check this again?

ghost · 2020-11-05T14:37:17Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2020-11-05T14:40:09Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/volta/app

…-tx' into feature/#1561-outgoing-as-custom-tx

ghost · 2020-11-05T16:37:12Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2020-11-05T16:39:46Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/volta/app

Now it directly checks agains the list of nftAssets on the store

ghost · 2020-11-05T17:46:14Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/volta/app

ghost · 2020-11-05T17:47:34Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2020-11-05T18:25:40Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2020-11-05T18:25:59Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/volta/app

ghost · 2020-11-06T16:43:47Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2020-11-06T16:46:58Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/volta/app

ghost · 2020-11-09T21:26:26Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/volta/app

ghost · 2020-11-09T21:27:04Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2020-11-10T18:20:32Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2020-11-10T18:20:54Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/volta/app

francovenica · 2020-11-10T19:44:54Z

Safes I used for testing:
0x9913B9180C20C6b0F21B6480c84422F6ebc4B808
0xD0BA3370F0cb4F2baB99c5CB6B2E6e3013682720 (this one was created for this PR)

I had an issue where the outgoing Tx of the collectible was labeled as Cancelled for a while. So I repeated the process 3 more times and I didn't had that issue anymore

I saw other weird stuff that are unrelated to the ticket, like the safe not recognizing me as owner for a few secs.
I talked with Agu Pane and it seems that is all related to the backend.

Since the issue of the outging tx for collectibles is not being labeled as "Custom Tx" anymore I can pass this ticket, and I'll test the feature further when 1574 reaches to Final QA

ghost · 2020-11-10T20:03:02Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2020-11-10T20:03:20Z

Travis automatic deployment:
https://pr1567--safereact.review.gnosisdev.com/volta/app

Agupane added 3 commits November 3, 2020 10:23

Refactor getTxData

927d772

Add SAFE_TRANSFER_FROM in SAFE_METHODS_NAMES

6951646

Adds check on isSendERC721Transaction for erc721 send

162df8b

Agupane self-assigned this Nov 3, 2020

Agupane requested review from dasanra, fernandomg and francovenica November 3, 2020 15:29

Agupane marked this pull request as ready for review November 3, 2020 15:30

Merge branch 'development' into feature/#1561-outgoing-as-custom-tx

3c17d80

Merge branch 'development' of https://github.com/gnosis/safe-react in…

50c1f64

…to feature/#1561-outgoing-as-custom-tx

fernandomg suggested changes Nov 4, 2020

View reviewed changes

Merge branch 'development' into feature/#1561-outgoing-as-custom-tx

167124d

Agupane added 4 commits November 5, 2020 13:16

Adds TOKEN_TRANSFER_METHODS_NAMES types

84d59c8

Replace type SAFE_TRANSFER_FROM

09c172b

Merge remote-tracking branch 'origin/feature/#1561-outgoing-as-custom…

c8dc2a6

…-tx' into feature/#1561-outgoing-as-custom-tx

Fix import

7522e2e

Agupane added 3 commits November 5, 2020 14:20

Adds nftAssetsListAddressSelector

f58417c

Remove txCode and knownTokens from isSendERC721Transaction

90848c9

Now it directly checks agains the list of nftAssets on the store

Refactor ENS_TOKEN_CONTRACT usage check

4715dc6

Add TODO

c2ddb71

fernandomg mentioned this pull request Nov 5, 2020

(Fix) Support CryptoKitties NFTs #1574

Merged

Agupane requested a review from fernandomg November 6, 2020 10:54

dasanra approved these changes Nov 6, 2020

View reviewed changes

Add return for ENS symbol

3e3a414

fernandomg approved these changes Nov 6, 2020

View reviewed changes

Merge branch 'development' into feature/#1561-outgoing-as-custom-tx

db5823c

Merge branch 'development' into feature/#1561-outgoing-as-custom-tx

f3ce564

Merge branch 'development' into feature/#1561-outgoing-as-custom-tx

1c7afd9

francovenica approved these changes Nov 10, 2020

View reviewed changes

Merge branch 'development' into feature/#1561-outgoing-as-custom-tx

4dc1452

Agupane merged commit ca73200 into development Nov 10, 2020

github-actions bot locked and limited conversation to collaborators Nov 10, 2020

fernandomg deleted the feature/#1561-outgoing-as-custom-tx branch November 10, 2020 20:30

Conversation

Agupane commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions bot commented Nov 3, 2020

Uh oh!

github-actions bot commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ESLint Summary View Full Report

Uh oh!

ghost commented Nov 3, 2020

Uh oh!

ghost commented Nov 3, 2020

Uh oh!

ghost commented Nov 4, 2020

Uh oh!

ghost commented Nov 4, 2020

Uh oh!

fernandomg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fernandomg Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

Agupane Nov 6, 2020

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 5, 2020

Uh oh!

ghost commented Nov 5, 2020

Uh oh!

ghost commented Nov 5, 2020

Uh oh!

ghost commented Nov 5, 2020

Uh oh!

ghost commented Nov 5, 2020

Uh oh!

ghost commented Nov 5, 2020

Uh oh!

ghost commented Nov 5, 2020

Uh oh!

ghost commented Nov 5, 2020

Uh oh!

ghost commented Nov 6, 2020

Uh oh!

ghost commented Nov 6, 2020

Uh oh!

ghost commented Nov 9, 2020

Uh oh!

ghost commented Nov 9, 2020

Uh oh!

ghost commented Nov 10, 2020

Uh oh!

ghost commented Nov 10, 2020

Uh oh!

francovenica commented Nov 10, 2020

Uh oh!

ghost commented Nov 10, 2020

Uh oh!

ghost commented Nov 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Agupane commented Nov 3, 2020 •

edited

Loading

github-actions bot commented Nov 3, 2020 •

edited

Loading