Skip to content
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

WIP:Feat/support keystone #801

Merged
merged 15 commits into from
Oct 17, 2024
Merged

WIP:Feat/support keystone #801

merged 15 commits into from
Oct 17, 2024

Conversation

ZhenQian-keystone
Copy link
Contributor

1、add keystone connect wallet support

2、add keystone sign tx support

@ZhenQian-keystone ZhenQian-keystone requested a review from a team as a code owner September 3, 2024 02:11
@ZhenQian-keystone ZhenQian-keystone requested review from allenan, tyler-whitman and Perronef5 and removed request for a team September 3, 2024 02:11
@ZhenQian-keystone ZhenQian-keystone changed the title WIP: Feat/support keystone [WIP]Feat/support keystone Sep 3, 2024
@ZhenQian-keystone ZhenQian-keystone changed the title [WIP]Feat/support keystone WIP:Feat/support keystone Sep 3, 2024
Perronef5
Perronef5 reviewed Sep 3, 2024
View reviewed changes
src/App.tsx Outdated Show resolved Hide resolved
Perronef5
Perronef5 reviewed Sep 3, 2024
View reviewed changes
package.json Show resolved Hide resolved
Perronef5
Perronef5 reviewed Sep 3, 2024
View reviewed changes
Copy link
Contributor

@Perronef5 Perronef5 left a comment

Choose a reason for hiding this comment

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

Overall PR is looking solid just a few comments here and there. Can we get a demo of how this all looks for an end to end flow. Thanks!

@ZhenQian-keystone
Copy link
Contributor Author

Overall PR is looking solid just a few comments here and there. Can we get a demo of how this all looks for an end to end flow. Thanks!

Thank you for your reply. I will make the modifications one by one. We will record a video and send it to the Telegram group.

ChewingGlass
ChewingGlass requested changes Sep 6, 2024
View reviewed changes
src/features/keystone/KeystoneAccountAssignScreen.tsx Outdated Show resolved Hide resolved
src/storage/cloudStorage.ts Outdated Show resolved Hide resolved
Perronef5
Perronef5 requested changes Oct 3, 2024
View reviewed changes
Copy link
Contributor

@Perronef5 Perronef5 left a comment

Choose a reason for hiding this comment

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

Tested this locally and saw a couple issues:

  1. When you scan a QR code that is not expected when connecting your keystone the app crashes. We need to handle this crash. (Happy path works as expected.)
  2. Unable to remove keystone wallet from within the settings.

Once these are fixed happy to test again 🙂

@ZhenQian-keystone
Copy link
Contributor Author

Tested this locally and saw a couple issues:

  1. When you scan a QR code that is not expected when connecting your keystone the app crashes. We need to handle this crash. (Happy path works as expected.)
  2. Unable to remove keystone wallet from within the settings.

Once these are fixed happy to test again 🙂

1、I fixed the issue where the app would crash when scanning an unknown QR code. I also added an alert to give users a heads up.

2、I tried to reproduce the problem with removing wallets on iOS devices, but it's working fine for me. Could you give me more details about what's going wrong? I'm having trouble seeing the issue on my end.

Perronef5
Perronef5 reviewed Oct 16, 2024
View reviewed changes
Copy link
Contributor

@Perronef5 Perronef5 left a comment

Choose a reason for hiding this comment

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

Tested your most recent changes and working well. Branch needs a rebase then I can approve 👍

@ZhenQian-keystone
Copy link
Contributor Author

Tested your most recent changes and working well. Branch needs a rebase then I can approve 👍

done

@Perronef5 Perronef5 merged commit e9a4af5 into helium:main Oct 17, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Perronef5 Perronef5 Perronef5 approved these changes

@allenan allenan Awaiting requested review from allenan allenan is a code owner automatically assigned from helium/app-team

@tyler-whitman tyler-whitman Awaiting requested review from tyler-whitman tyler-whitman is a code owner automatically assigned from helium/app-team

@ChewingGlass ChewingGlass Awaiting requested review from ChewingGlass

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ZhenQian-keystone @Perronef5 @ChewingGlass