Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

(Fix) Tx Table fails to display OutgoingTx data #799

Merged
merged 4 commits into from
Apr 24, 2020

Conversation

fernandomg
Copy link
Contributor

This is @Agupane finding, we were working on fixes.

  • wrong parameters order: c887c71
  • wrong logic for some sendTokens: cdcdd07
  • invalid display of txs amount with long decimals: 55eee73

@ghost
Copy link

ghost commented Apr 23, 2020

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

Copy link
Contributor

@francovenica francovenica left a comment

Choose a reason for hiding this comment

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

Outgoing tx looks fine

@ghost
Copy link

ghost commented Apr 24, 2020

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

@mmv08 mmv08 merged commit d91f403 into development Apr 24, 2020
@mmv08 mmv08 deleted the fix/txTable-outgoingTx branch April 24, 2020 14:02
@francovenica
Copy link
Contributor

@lukasschor Just to clarify. We should see the short notation (<0.0001) in both, the tx and the overview? or just in the tx row?
Also, it is not working for ETH
@Agupane @fernandomg

image

@lukasschor
Copy link
Member

Do we show the exact number in the transaction details? If yes, then I think this should be fine.

So it should ideally show the exact number in the details. But this would not be a release blocker for me. Users can always go to etherscan, but then we should fix it later.

@lukasschor
Copy link
Member

The fact that it does not shorten for ETH is also fine for now.

@francovenica
Copy link
Contributor

@lukasschor incoming tx should also use the short notation? I think it should, for consistency
image

@lukasschor
Copy link
Member

@fernandomg

If possible could you change the behavior to

  • Short notation in the overview for all transactions (incoming, outgoing, ETH/ERC-20)

  • Long notation in the transaction details for all transactions

If we can merge it by Monday we can of course include it in the release. But if there's not enough time to do testing, afterwards I would leave it out for the release. We can and just make a hotfix (2.0.1.) afterwards.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants