Skip to content

frontend: type keystores #2307

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.

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

Closed

Conversation

thisconnect
Copy link
Collaborator

@thisconnect thisconnect commented Oct 24, 2023

This should only have been a PR to type keystore api, but ran into unknown account error, see 2nd and 3rd commit. 3rd commit fixes unpluging the device for me.

activeSidebar,
sidebarStatus,
} = this.props;
console.log('deviceIDs/keystores/accounts', deviceIDs, keystores, accounts);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

when an error occurs, keystores is empty, but accounts still has the accounts

Copy link
Collaborator Author

@thisconnect thisconnect Oct 24, 2023

Choose a reason for hiding this comment

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

Screen Shot 2023-10-24 at 09 16 42

cc @Beerosagos could you check in which order the events are pushed to the frontend?

EDIT: it looks like it is pushing in the wrong order, but it could be the frontend that is doing it funny.
What I did NOT expect is that there is no keystores but there is still accounts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems to me that the notification order from the backend is 1. keystore 2. accounts 3. device.
See in particular https://github.com/digitalbitbox/bitbox-wallet-app/blob/master/backend/backend.go#L552-L555 that emits the keystore event before the account one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Opened a separate PR to fix #2311

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thank you ❤️

I'll drop the last commit in this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

since this PR error is not related to this PR I'll update the last commit message, and think we could merge this anyway. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rebased onto staging-watchonly here: #2389

I wonder if the error when unpluging a device still occurs

cc @benma just FYI.

@thisconnect thisconnect force-pushed the frontend-type-keystores branch 3 times, most recently from 3597586 to bf74fe2 Compare October 24, 2023 15:00
@thisconnect thisconnect force-pushed the frontend-type-keystores branch from bf74fe2 to 56ddd9f Compare October 25, 2023 06:12
@thisconnect
Copy link
Collaborator Author

rebased

@thisconnect thisconnect force-pushed the frontend-type-keystores branch from 56ddd9f to 3d3e16d Compare October 25, 2023 11:58
Used mounted hook to only update AddBuyReceiveOnEmptyBalances state
if the component is actually mounted.
Copy link
Collaborator

@Beerosagos Beerosagos left a comment

Choose a reason for hiding this comment

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

tACK!

@thisconnect
Copy link
Collaborator Author

closed in favor of #2389

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

Successfully merging this pull request may close these issues.

2 participants