core icon indicating copy to clipboard operation
core copied to clipboard

Fix/keep nft when transaction fail

Open tommasini opened this issue 3 years ago • 5 comments

Keep nft when transaction fail -> when the NFT is being transacted, was needed some changes

  • Was added two new props to the collectible interface in CollectibleController, for when a collectible is being transacted we have can know about it and change the UI according to that.

Description

Itemize the changes you have made into the categories below

  • CHANGED:

    • Changed the Collectible Props to have more two optional props
  • ADDED:

    • It was added 3 functions:
    • One to get the collectible transaction status, it will return if the collectible is being transacted and the id of that transaction
    • One to update the properties transactionId and isTransacting of the collectible
    • One to reset the transaction status of the collectible that updates the prop isTransacting for false and the transactionId to an empty string. It returns true if have success, false if have not

Checklist

  • [x] Tests are included if applicable
  • [x] Any added code is fully documented

Issue

Resolves #https://github.com/MetaMask/metamask-mobile/issues/2784

It will have an impact on this PR of metamask mobile

tommasini avatar Apr 01 '22 12:04 tommasini

Pinging @adonesky1 in case the Extension has the same issue as Mobile.

Issue: The app removes the collectible when the transaction is submitted instead of waiting for it to be confirmed

gantunesr avatar Apr 25 '22 03:04 gantunesr

Pinging @adonesky1 in case the Extension has the same issue as Mobile. Issue: The app removes the collectible when the transaction is submitted instead of waiting for it to be confirmed

Sorry coming to this late... wondering if you had considered using the method checkAndUpdateSingleCollectibleOwnershipStatus. In the extension the way we update ownership of a collectible after an attempted send can be found here. Basically when we get a notification from the transaction controller that a transaction status has updated we check if it was a transferFrom token method and get the tokenId and contractAddress and send them to checkAndUpdateSingleCollectibleOwnershipStatus to see if the user still owns the collectible after the transaction and update the state accordingly.

@tommasini @gantunesr

adonesky1 avatar Apr 28 '22 18:04 adonesky1

Hey, @adonesky1 Thank you so much for the review and for bringing these methods up. I was thinking about how to implement those methods into our issue, but the methods that are added in this PR are to make be able to make some changes in the UI when the NFT is being transacted, so I would have always to update the collectible with the transactionId. To remove the collectible we use the method removeCollectible when the tx is confirmed

Here is one small demo of the final result of what we accomplish https://user-images.githubusercontent.com/46944231/167123888-30dc816d-d8d5-494b-9208-0c71c48f43ff.mov

tommasini avatar May 06 '22 11:05 tommasini

@tommasini @Gudahtt I'm sorry I don't understand why we need to associate a transactionId with the collectible in the CollectiblesController state and these additional methods to achieve your goals here. Could we jump on a call and discuss sometime next week?

adonesky1 avatar May 06 '22 21:05 adonesky1

@tommasini added some comments/change requests! Let me know if you have any questions!

adonesky1 avatar May 13 '22 14:05 adonesky1