Skip to content

Custodial account UI #471

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jan 13, 2023
Merged

Custodial account UI #471

merged 3 commits into from
Jan 13, 2023

Conversation

@itsrachelfish
Copy link
Contributor

@itsrachelfish itsrachelfish commented Jan 4, 2023

This PR updates the custom session UI to include custodial accounts.

image

Depends on #451

@dstrukt dstrukt self-requested a review January 5, 2023 03:10
@itsrachelfish itsrachelfish force-pushed the custodial-account-ui branch 2 times, most recently from 03edf77 to 0823147 Compare January 6, 2023 20:53
@itsrachelfish itsrachelfish marked this pull request as ready for review January 6, 2023 20:53
guggero
guggero reviewed Jan 9, 2023
View reviewed changes
Copy link
Contributor

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Yeah, very cool!
Gave the UI a quick spin and here is one thing I ran into:
If I don't specify a balance, the backend gives me an error (RPCS: [/litrpc.Accounts/CreateAccount]: unable to create account: a new account cannot have balance of 0) but the frontend just stays the way it was before. The console log shows the error, but there is no toast/message being displayed in the UI.

Other than that, everything looks and works great, awesome work 🎉

jamaljsr
jamaljsr reviewed Jan 9, 2023
View reviewed changes
Copy link
Member

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

Overall, this looks great. I also saw the issue @guggero mentioned.

I'll reiterate here the suggestions I made over chat last week.

  1. Keep the QR code icon enabled for custodial sessions
  2. Add a custodial flag to the encoded data used in the TW url. Just be mindful of backwards compatibility for apps that may already use the existing QR code, like Zeus.

@levmi levmi requested review from guggero and jamaljsr January 10, 2023 18:39
jamaljsr
jamaljsr approved these changes Jan 10, 2023
View reviewed changes
Copy link
Member

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

tACK LGTM 👍

All clear from me!

guggero
guggero approved these changes Jan 11, 2023
View reviewed changes
Copy link
Contributor

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@itsrachelfish itsrachelfish force-pushed the custom-connect-ui branch 2 times, most recently from 863f5b0 to 4665cf3 Compare January 12, 2023 00:36
@itsrachelfish
Copy link
Contributor Author

itsrachelfish commented Jan 12, 2023

Had to rebase this branch after updating the custom-connect-ui branch with the latest changes from master

@guggero
Copy link
Contributor

guggero commented Jan 13, 2023

Can we merge this into the custom-connect-ui branch and then merge #451 as a whole? Looks like we got a bunch of reviews already (or do we want to wait for @dstrukt's review?).

@jamaljsr
Copy link
Member

jamaljsr commented Jan 13, 2023

Agreed @guggero

@jamaljsr jamaljsr merged commit 797689c into custom-connect-ui Jan 13, 2023
@jamaljsr jamaljsr deleted the custodial-account-ui branch January 13, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jamaljsr jamaljsr jamaljsr approved these changes

@dstrukt dstrukt Awaiting requested review from dstrukt

+2 more reviewers

@guggero guggero guggero approved these changes

@hexafluoride hexafluoride hexafluoride approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@itsrachelfish itsrachelfish

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@itsrachelfish @guggero @jamaljsr @hexafluoride