-
Notifications
You must be signed in to change notification settings - Fork 168
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
major: HDKD feature #431
major: HDKD feature #431
Conversation
…er into sveta-hanwen-hdkd
…ity-signer into sveta-hanwen-hdkd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review (will get back at it later today)
This looks awesome and work very very well, I've had a hard time finding bugs 🎉
tested:
- ✔️ Migration from master to this branch with both ETH and Kusama dev accounts
- ✔️ Sign and Tx on Eth kovan successful
- ✔️ Multipart and extrinsics on Kusama dev successful
Found one problem, some nits, and general comments:
- deleting an account doesn't require the pin
- (nit) we hard-coded the word "extrinsic" although this doesn't apply to Etherrm: screenshot
- (nit) Ethereum message signing should probably have some margin with the account card: screenshot
General feedback (not necessarily to be addressed in this PR):
- Right now the structure is probably super clear in your mind. it'd be great to have a comment exlaining the structure like (this is nonsense, but just to get an idea)
IdentityMap = {
['//hard/path']: {
"meta": {
"name": "account name"
}
...
}
}
- style in components clutter them and make them harder to read. I'd advice in the future to use global style variables.
data={identitiesToShow} | ||
renderItem={renderNonSelectedIdentity} | ||
keyExtractor={item => item.encryptedSeed} | ||
style={{ paddingVertical: identities.length > 5 ? 8 : 0 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (and the same check >5
2 lines down) looks sketchy 👀
Why 5
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just talked with Sveta, 8 is the Height of the scroll container.
If there are more than 5 identities, the list will become scrollable.
there will be shadow to indicate that there's more and some other details.
So I think it is decide by the smallest screen, just checked on iPhone SE. Also tried on my big Android screen, there are more margin, but I think it is OK for now. We could programmatically change that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to discuss it separately, I think that would need discussion multiple possibilities together with Sveta, I will open an issue later this afternoon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have add the current data schema in the description, hope that helps! And I also agree with group styling, for example, there are lots of uniform styling in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested again extensively (ETH and Kusama dev), multi-identity, recovering accounts as well, it just works.
🎉 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍻🍻🍻Many thanks, @goldsteinsveta @hanwencheng!
src/components/SecurityHeader.js
Outdated
NetInfo.addEventListener(state => { | ||
setIsConnected(state.isConnected); | ||
}); | ||
return subscribe(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems this is missing an unsubscribe() on component unmount?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this, but it's "just" a typo imho. It should be named unsubscribed, and this will be called when the component unmounts (because it's what the useEffect
returns)
It was named this.unsubscribe
previously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is kind of strange because this subscribe
directly return an unsubscribe function. Probably name it as unsubscribe
.
Here I wrapped it in function so it may be different, I will just extract them out.
sample usage from NetInfo
// Subscribe
const unsubscribe = NetInfo.addEventListener(state => {
console.log("Connection type", state.type);
console.log("Is connected?", state.isConnected);
});
// Unsubscribe
unsubscribe();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how I'm used to see subscriptions. Polkadot-js api does the same. So to do things right we can rename it yes, but I think the logic is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, let's just refactor to save "future us" unnecessary confusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now it is even cleaner:
useEffect(
() =>
NetInfo.addEventListener(state => {
setIsConnected(state.isConnected);
}),
[]
);
…er into sveta-hanwen-hdkd
YJ also approved the PR with today's chat |
indeed 💃 |
So I am thinking how to collaborate together with @goldsteinsveta on this issue, we may work and track on this branch and record the progress here.
Design
References to design: https://www.figma.com/file/4odbGhaJ3OTN2ZQb52uvIy/Signer_UI-2.0?node-id=1760%3A148
Data Schemas
The accountId with Ethereum saved into storage is now with Uppercase. But we still recognize QR code with lower case account.
Seed Unlock Refractor
Also include the refactoring of unlocking logic, the setPin and unLock will become a function to call the Pin input screen like the following:
With new pin code input screen, the pin code input mode will be changed.
Also closes #311, closes #324, closes #448, closes #315 , closes #432 , closes #443, closes #404
Tested Cases
Manual
Automat
yarn e2e:android
yarn e2e:ios
yarn test
yarn test-rust
Not 100% Sure Thing
dcd7477 : Reset
preHash
incleanup()
function in ScannerStore.Problem Tracking