Siwe 2.0 Sign panel and details dialog #20168

Pavneet-Sing · 2023-09-14T10:04:30Z

Resolves brave/brave-browser#32863

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:

npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint

Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

SIWE.webm

Pavneet-Sing · 2023-09-14T10:06:41Z

android/java/org/chromium/chrome/browser/crypto_wallet/fragments/dapps/SiweMessageFragment.java

+            jsonObject.put("scheme", origin.scheme);
+            jsonObject.put("host", origin.host);
+            jsonObject.put("port", origin.port);


Info: LMK if we should also localise these URL titles (scheme, host, port).

I think we are fine with how it is now, but I will let string reviewers to comment on it.

.../java/org/chromium/chrome/browser/crypto_wallet/adapters/TwoLineItemRecyclerViewAdapter.java

SergeyZhukovsky · 2023-09-15T02:08:47Z

I would do more space between text on sign in message.
We can base the PR off master since the original PR is already merged.

Pavneet-Sing · 2023-09-15T04:41:55Z

I would do more space between text on sign in message.
We can base the PR off master since the original PR is already merged.

Will add some padding between the items.
This PR was based upon master (with origin PR changes) before creation, don't see any conflicts etc though LMK if there's anything missing.

Padding updated: 585cb2d

AlexeyBarabash

lgtm

SergeyZhukovsky

++

kdenhartog

few comments for the code, still need to do some dynamic testing with this

android/java/res/layout/tx_details_bottom_sheet.xml

android/java/res/layout/item_fragment_two_line_divider_item.xml

android/java/org/chromium/chrome/browser/crypto_wallet/util/AddressUtils.java

mkarolin

strings++

kdenhartog · 2023-09-19T23:15:59Z

Screen.Recording.2023-09-20.at.11.12.36.AM.mov

The "see details" tab is currently not displaying all of the necessary details nor is it scrollable. Can we fix this and then no other security concerns.

Pavneet-Sing · 2023-09-20T07:00:26Z

Screen.Recording.2023-09-20.at.11.12.36.AM.mov
The "see details" tab is currently not displaying all of the necessary details nor is it scrollable. Can we fix this and then no other security concerns.

I just compared the above with desktop and I don't see any missing details, could you be more specific about what details are missing? though I think it is same as desktop so probably we are good here?

Seems like we missed the not before and requestId fields in the origin SIWE PR so thanks for noticing it. Updated this PR with the changes d2777a6

SIWE2.webm

BTW, the list is scroll-able but there are no more details to scroll. Dialog is able to show all the details in the current window so scroll is not required. (though you can observe the upwards movement in the details list but there are no more items to show hence no scrolling,)

kdenhartog · 2023-09-21T11:33:17Z

Sounds good, I'll build the latest overnight on CI and check it in the morning

Pavneet-Sing · 2023-09-25T03:39:54Z

Sounds good, I'll build the latest overnight on CI and check it in the morning

Did you get a chance to test this?

kdenhartog · 2023-09-25T06:09:43Z

Sorry about the delay, started the build and never got back to testing until just now. I'm not seeing this fixed though.
Link to the build I ran. which is relying on the latest revision of d2777a6 (#20168). Are you seeing the same thing I'm seeing? I'm just realizing I didn't state what I saw missing last time 🤦🏻 . The particular attribute I'm thinking about here is the Resources property. This should still be viewable in both the consolidated and detailed view like the statement is.

Also, it would be ideal to make sure the origin property didn't wrap host value within the JSON. Right now the host is split in a way that makes it unclear if this is very long.

Screen.Recording.2023-09-25.at.7.06.31.PM.mp4

Pavneet-Sing · 2023-09-25T08:40:04Z

Sorry about the delay, started the build and never got back to testing until just now. I'm not seeing this fixed though. Link to the build I ran. which is relying on the latest revision of d2777a6 (#20168). Are you seeing the same thing I'm seeing? I'm just realizing I didn't state what I saw missing last time 🤦🏻 . The particular attribute I'm thinking about here is the Resources property. This should still be viewable in both the consolidated and detailed view like the statement is.

Also, it would be ideal to make sure the origin property didn't wrap host value within the JSON. Right now the host is split in a way that makes it unclear if this is very long.

Screen.Recording.2023-09-25.at.7.06.31.PM.mp4

Thanks for the quick response and details. Per comment and DM discussion, I have added the resources and host in the details dialog b9231db . PTAL.

SIWE.webm

kdenhartog

We're going to push origin changes for now and address in brave/brave-browser#33227 instead. Rest LGTM

Pavneet-Sing added 7 commits September 14, 2023 14:53

implement SIWE 2.0 sign dialog

77c43cb

Implement SIWE details section

df73d6e

Use domain models

6c682a0

Show origin

2d8ab8f

Implement SIWE details dialog and formatting

21bb5eb

Update layout design

bb046f2

Clean up

c8cbd6e

Pavneet-Sing added CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS CI/skip-windows-x86 CI/skip-windows-x64 Do not run CI builds for Windows x64 unused-CI/skip-linux-x64 Do not run CI builds for Linux x64 labels Sep 14, 2023

Pavneet-Sing requested review from SergeyZhukovsky, kdenhartog and AlexeyBarabash September 14, 2023 10:04

Pavneet-Sing self-assigned this Sep 14, 2023

Pavneet-Sing requested a review from a team as a code owner September 14, 2023 10:04

github-actions bot added the feature/web3/wallet label Sep 14, 2023

Pavneet-Sing commented Sep 14, 2023

View reviewed changes

SergeyZhukovsky reviewed Sep 15, 2023

View reviewed changes

.../java/org/chromium/chrome/browser/crypto_wallet/adapters/TwoLineItemRecyclerViewAdapter.java Outdated Show resolved Hide resolved

AlexeyBarabash approved these changes Sep 15, 2023

View reviewed changes

Minor review changes

585cb2d

Pavneet-Sing requested a review from SergeyZhukovsky September 15, 2023 06:42

Add spacing

0eb9873

SergeyZhukovsky approved these changes Sep 15, 2023

View reviewed changes

kdenhartog reviewed Sep 18, 2023

View reviewed changes

android/java/res/layout/tx_details_bottom_sheet.xml Show resolved Hide resolved

android/java/res/layout/item_fragment_two_line_divider_item.xml Show resolved Hide resolved

android/java/org/chromium/chrome/browser/crypto_wallet/util/AddressUtils.java Show resolved Hide resolved

mkarolin approved these changes Sep 19, 2023

View reviewed changes

Add missing SIWE detail fields

d2777a6

Pavneet-Sing added 3 commits September 25, 2023 14:03

Fix item spacing

a07b26e

Show resources and host in details dialog

b9231db

Fix formatting

893eb45

Pavneet-Sing requested a review from kdenhartog September 25, 2023 08:40

Remove host field

25420e1

kdenhartog approved these changes Sep 25, 2023

View reviewed changes

Pavneet-Sing enabled auto-merge (squash) September 25, 2023 09:07

Pavneet-Sing merged commit 1de9f42 into master Sep 25, 2023

Pavneet-Sing deleted the siwe-2.0-android branch September 25, 2023 10:33

github-actions bot added this to the 1.60.x - Nightly milestone Sep 25, 2023

This was referenced Oct 31, 2023

Update android SIWE flow per spec brave/brave-browser#32863

Closed

Sign-In with Ethereum (Android UI) brave/brave-browser#32077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Siwe 2.0 Sign panel and details dialog #20168

Siwe 2.0 Sign panel and details dialog #20168

Pavneet-Sing commented Sep 14, 2023 •

edited

Loading

Pavneet-Sing Sep 14, 2023

SergeyZhukovsky Sep 15, 2023

SergeyZhukovsky commented Sep 15, 2023

Pavneet-Sing commented Sep 15, 2023 •

edited

Loading

AlexeyBarabash left a comment

SergeyZhukovsky left a comment

kdenhartog left a comment

mkarolin left a comment

kdenhartog commented Sep 19, 2023

Pavneet-Sing commented Sep 20, 2023 •

edited

Loading

kdenhartog commented Sep 21, 2023

Pavneet-Sing commented Sep 25, 2023

kdenhartog commented Sep 25, 2023 •

edited

Loading

Pavneet-Sing commented Sep 25, 2023 •

edited

Loading

kdenhartog left a comment

Siwe 2.0 Sign panel and details dialog #20168

Siwe 2.0 Sign panel and details dialog #20168

Conversation

Pavneet-Sing commented Sep 14, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Pavneet-Sing Sep 14, 2023

Choose a reason for hiding this comment

SergeyZhukovsky Sep 15, 2023

Choose a reason for hiding this comment

SergeyZhukovsky commented Sep 15, 2023

Pavneet-Sing commented Sep 15, 2023 • edited Loading

AlexeyBarabash left a comment

Choose a reason for hiding this comment

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

kdenhartog left a comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

kdenhartog commented Sep 19, 2023

Pavneet-Sing commented Sep 20, 2023 • edited Loading

kdenhartog commented Sep 21, 2023

Pavneet-Sing commented Sep 25, 2023

kdenhartog commented Sep 25, 2023 • edited Loading

Pavneet-Sing commented Sep 25, 2023 • edited Loading

kdenhartog left a comment

Choose a reason for hiding this comment

Pavneet-Sing commented Sep 14, 2023 •

edited

Loading

Pavneet-Sing commented Sep 15, 2023 •

edited

Loading

Pavneet-Sing commented Sep 20, 2023 •

edited

Loading

kdenhartog commented Sep 25, 2023 •

edited

Loading

Pavneet-Sing commented Sep 25, 2023 •

edited

Loading