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.

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

refactor/keys: refactor ClientKeys and AppKeys to use full IDs #1058

Merged
merged 2 commits into from Nov 22, 2019

Conversation

@m-cat
Copy link
Contributor

m-cat commented Nov 1, 2019

No description provided.

@m-cat m-cat requested a review from nbaksalyar as a code owner Nov 1, 2019
@m-cat m-cat force-pushed the m-cat:full-client-keys branch 3 times, most recently from 428776d to 6ef4612 Nov 5, 2019
@m-cat m-cat force-pushed the m-cat:full-client-keys branch from 6ef4612 to 385e9ef Nov 15, 2019
@m-cat m-cat changed the title WIP refactor/keys: refactor ClientKeys and AppKeys to use full IDs Nov 15, 2019
@m-cat m-cat force-pushed the m-cat:full-client-keys branch 4 times, most recently from 6997360 to d841f3c Nov 20, 2019
Copy link
Member

lionel1704 left a comment

Nice work with this @m-cat!
Just a couple of small comments and one question. Do we still need to point to your fork of safe-nd?
We don't need the KeyPair struct, so is there any other change (other than re-arrangement of modules) that we need from maidsafe/safe-nd#123 ?

client_keys,
owner_key,
app_keys,
public_key,

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Nov 21, 2019

Member

Can we call this owner_key instead?
Just for clarity.

let owner_key = c3.owner_key();
let keys = AppKeys::random(owner_key);
let public_id = c3.public_id();
let keys = AppKeys::new(unwrap!(public_id.client_public_id()).clone());

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Nov 21, 2019

Member

If this unwrap! cannot be removed can we please justify it in a comment.

@m-cat

This comment has been minimized.

Copy link
Contributor Author

m-cat commented Nov 21, 2019

Yes, this PR depends on the safe-nd changes. Among other things, we create Keypairs in tests to get public keys -- technically not needed, but it is convenient.

@m-cat m-cat force-pushed the m-cat:full-client-keys branch 2 times, most recently from a6fd4a8 to 08c43c6 Nov 21, 2019
Copy link
Member

lionel1704 left a comment

Looks good, thanks!

@lionel1704 lionel1704 merged commit ddc81ba into maidsafe:master Nov 22, 2019
8 of 9 checks passed
8 of 9 checks passed
Rustfmt-Clippy
Details
build-android (armv7-linux-androideabi, prod)
Details
build-android (armv7-linux-androideabi, dev)
Details
build-android (x86_64-linux-android, prod)
Details
build-android (x86_64-linux-android, dev)
Details
Test (ubuntu-latest)
Details
Test (windows-latest)
Details
Travis CI - Pull Request Build Failed
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.