search

symbol / mobile-wallet

Symbol Mobile Wallet
Apache License 2.0
8 stars 14 forks source link

feat: Aggregate Transaction View. Changed AggregateBonded signing flow. Inner transaction presentation and Transaction Graphic. #240

Closed OlegMakarenko closed 2 years ago

OlegMakarenko commented 2 years ago

image

cryptoBeliever commented 2 years ago

  1. For Samsung S20 I can' see the graphic fully. I can't scroll horizontally too: image

  2. Improvement proposition: Would be nice to add a swipe left/right for switching tabs (About transaction/Graphic/Inner transactions).

  3. For multisig creation transaction I can see Received cosignatures as n/a

http://explorer.symbolblockchain.io/transactions/965715E9CBFE67EF2E5CF2AE5F38B18A7974385EE82A9AA16FD62ED1B9F6A638

image

For aggregate complate receive cosignatures is empty (only label)

  1. Are all transaction types supported? For address alias I can see not supported type:

image

Same for secret lock. Support of all tx types does apply only to aggregate?

Edit (OlegMakarenko): scale down the screenshots

lgtm-com[bot] commented 2 years ago

This pull request introduces 17 alerts and fixes 5 when merging 38236c5fb26bb20c894939e10c8870eb1f14beeb into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 17 alerts and fixes 5 when merging 2df9725fc27892df56b746357c48beb6dbc6c610 into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 17 alerts and fixes 5 when merging dcfe6594a3be750ff88a4cc77dbe1ded636b3fef into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 17 alerts and fixes 5 when merging 3133abf85aafd1caef7d7f90481d165c0395edb7 into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 17 alerts and fixes 5 when merging 6790fd0a57a06706ecfe38283fabefc3db39f15c into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 17 alerts and fixes 5 when merging 8d2097d606e295f982a0fa172a8f719ea7715a6f into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 17 alerts and fixes 5 when merging d3c2886e26bc3171cf6f19d1e5971a72db62e3db into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 17 alerts and fixes 5 when merging a90f4e25aabb3357696dce0e4a7413933a5ea487 into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 17 alerts and fixes 5 when merging 2ca17d8185e408db1ea725ce0031ae279d1b39f8 into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

yilmazbahadir commented 2 years ago

Probably it'd be also good to get @AnthonyLaw and @bassemmagdy's reviews on this one.

lgtm-com[bot] commented 2 years ago

This pull request introduces 17 alerts and fixes 5 when merging 54e4fd5bbc972658406c50e6805aafaba46e1da2 into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 17 alerts and fixes 5 when merging 46de55c77612ae4a6ce50226557e2d1c9aa71131 into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 17 alerts and fixes 5 when merging e5902f1ecd29f996ded8202c13a849806017cda2 into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 17 alerts and fixes 5 when merging 5d102059087bcbfada078b379a6a64897cbd07b5 into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 11 alerts and fixes 6 when merging 730da5b5b1efce5005dbf33108111f125188a156 into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

AnthonyLaw commented 2 years ago

@OlegMakarenko You may want to disable sonar cloud for the line of code.

https://github.com/symbol/mobile-wallet/blob/5e563b6f80018693273c3f96ef4d9201b6bbb79a/src/store/network.js#L98

yilmazbahadir commented 2 years ago

@OlegMakarenko Can you please fix the LGTM issues? https://lgtm.com/projects/g/symbol/mobile-wallet/rev/pr-70333075811b7da820f168663f2d319cb8eb55d9

lgtm-com[bot] commented 2 years ago

This pull request introduces 5 alerts and fixes 6 when merging f1fdf4ec4c71abba45fc1693df1339997df94c7e into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 4 alerts and fixes 6 when merging d7011e402ab13f64198858d2c1ba0f1b69d39084 into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 1 alert and fixes 6 when merging 7d4c9916a121cce35a343b0a196e6e7d3a602c0c into 5e563b6f80018693273c3f96ef4d9201b6bbb79a - view on LGTM.com

new alerts:

fixed alerts:

cryptoBeliever commented 2 years ago

@yilmazbahadir @OlegMakarenko could you build APK for me? Looks like Jenkins agent is off again.

EDIT: got APK from Oleg

cryptoBeliever commented 2 years ago

@OlegMakarenko below my comments

  1. For key links (voting, address, VRF, node) and address/mosaic alias there is one icon no matter if this is LINK or UNLINK operation. In explorer for UNLINK we have a strike-through icon, for LINK it's not a strike-through. In a mobile wallet, it's always strike-through not matter if it's LINK or UNLINK.

image

  1. Transaction Details -> About transaction. For bonded aggregates "Received Cosigntures" are not presented correctly. It's always "n/a". Also in the desktop wallet, this field has: "Co-signatures received" label.

image

  1. For mosaic definition transactions there is a missing "divisibility" field.

  2. Secret proof/lock and Account metadata. Right side of arrow is not resolved to "You" if the address matches. For example: https://testnet.symbol.fyi/transactions/FEF92C4765139C9E4C89AFE614F9A75A815E59FCE15C4421562C80FEEE718301

image

  1. Mosaic global restriction:

    • There is missing "Previous restriction value" field
    • Restriction types are presented as operators for example =, but in explorer, it's word "Equal"

  2. Namespace registration / mosaic creation / has lock / secret locks:

    • Would be possible to present also human-readable values instead of only blocks? For example add 30d 0h 0m instead only value 86400?
    • it will require additional calls to present the estimated rental fee for mosaic and namespace? If not we can add that.

  3. "Mosaic address restriction". In explorer on the right side of arrow, we have "mosaic" icon. In mobile wallet it's target address.

https://testnet.symbol.fyi/transactions/0627321705D4D0424BA7EC796E3A621D85212860A9023C9DD3C78EC6E40884ED

image

  1. Target address is presented without dashes for mosaic address restriction, account/namespace/mosaic metadata). Same with Account address restriction / multisig modifications and address additions/deletions.

  2. Maybe we should change "Waiting for your signature" to Waiting for your review"?

  3. I think that we should present transactions unfold by default. The user should review all of them.

  4. Maybe we should allow user to quit here too by adding "Cancel"/"I don't recognize transaction" button? User can close by "x" button but it's not so visible and can force user to just sign because it's easier from UI part.

image

lgtm-com[bot] commented 2 years ago

This pull request introduces 1 alert and fixes 6 when merging 26986caccd38801b37d2fdb7665e7828f1eca39c into c8439488e28cb3ea83de6f44a52750d21c6f8b49 - view on LGTM.com

new alerts:

fixed alerts:

sonarcloud[bot] commented 2 years ago

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 24 Code Smells

No Coverage information No Coverage information
10.8% 10.8% Duplication

lgtm-com[bot] commented 2 years ago

This pull request introduces 1 alert and fixes 6 when merging 5683d12175a69b3aa820d619650364b1e84160ce into c8439488e28cb3ea83de6f44a52750d21c6f8b49 - view on LGTM.com

new alerts:

fixed alerts:

OlegMakarenko commented 2 years ago

@cryptoBeliever

For key links (voting, address, VRF, node) and address/mosaic alias there is one icon no matter if this is LINK or UNLINK operation. In explorer for UNLINK we have a strike-through icon, for LINK it's not a strike-through. In a mobile wallet, it's always strike-through not matter if it's LINK or UNLINK.

Fixed.

Transaction Details -> About transaction. For bonded aggregates "Received Cosigntures" are not presented correctly. It's always "n/a". Also in the desktop wallet, this field has: "Co-signatures received" label.

Fixed. Label text updated.

For mosaic definition transactions there is a missing "divisibility" field.

Fixed. The 0 values should be diplayed correctly.

Secret proof/lock and Account metadata. Right side of arrow is not resolved to "You" if the address matches. For example: https://testnet.symbol.fyi/transactions/FEF92C4765139C9E4C89AFE614F9A75A815E59FCE15C4421562C80FEEE718301

Fixed.

Mosaic global restriction: There is missing "Previous restriction value" field

Fixed. The 0 values should be diplayed correctly.

Restriction types are presented as operators for example =, but in explorer, it's word "Equal"

In the Explorer's Transaction Graphic it is also displayed as operators.

Namespace registration / mosaic creation / has lock / secret locks: Would be possible to present also human-readable values instead of only blocks? For example add 30d 0h 0m instead only value 86400? it will require additional calls to present the estimated rental fee for mosaic and namespace? If not we can add that.

Sounds good. Let's add it in the next PR (to the Explorer as well).

"Mosaic address restriction". In explorer on the right side of arrow, we have "mosaic" icon. In mobile wallet it's target address. https://testnet.symbol.fyi/transactions/0627321705D4D0424BA7EC796E3A621D85212860A9023C9DD3C78EC6E40884ED

Yes, we should fix it in the Explorer as well.

Target address is presented without dashes for mosaic address restriction, account/namespace/mosaic metadata). Same with Account address restriction / multisig modifications and address additions/deletions.

Fixed.

I think that we should present transactions unfold by default. The user should review all of them.

Added.

Maybe we should allow user to quit here too by adding "Cancel"/"I don't recognize transaction" button? User can close by "x" button but it's not so visible and can force user to just sign because it's easier from UI part.

The extra button will make the purple panel large, and it will be hard to review transaction

cryptoBeliever commented 2 years ago

@OlegMakarenko

I.

Restriction types are presented as operators for example =, but in explorer, it's word "Equal" In the Explorer's Transaction Graphic it is also displayed as operators.

Me: I mean restriction type in explorer it's as text (Equal) but in mobile, it's =:

https://testnet.symbol.fyi/transactions/B60BBCBF46E8015BF05A344A2D64957AD2C6D6C2F058207119608D3754005E75

image

II.

"Mosaic address restriction". In explorer on the right side of arrow, we have "mosaic" icon. In mobile wallet it's target address. https://testnet.symbol.fyi/transactions/0627321705D4D0424BA7EC796E3A621D85212860A9023C9DD3C78EC6E40884ED

Yes, we should fix it in the Explorer as well.

Me: It's not a mosaic icon in explorer and an address icon in mobile. How it should be presented?

III.

I think that we should present transactions unfold by default. The user should review all of them. Added.

Me: I meant here to show transaction details by default. Now user has to click to see details of single inner tx. What you think about it?

IV.

Maybe we should allow user to quit here too by adding "Cancel"/"I don't recognize transaction" button? User can close by "x" button but it's not so visible and can force user to just sign because it's easier from UI part.

The extra button will make the purple panel large, and it will be hard to review transaction

Me: I'm afraid people will accepting by default

V.

Maybe we should change "Waiting for your signature" to Waiting for your review"?

Me: What you think about it?

cryptoBeliever commented 2 years ago

Would be good to check on IOS if all looks good. @yilmazbahadir we need to push to TestFlight and ask @NikolaiB to verify.

yilmazbahadir commented 2 years ago

@cryptoBeliever Jenkins is failing to build the IOS and the android artifacts seem to be corrupted. Will try to do it manually today.