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

feat/keys: add `Keypair` and remove bls methods for full IDs #123

Merged
merged 3 commits into from Nov 21, 2019

Conversation

@m-cat
Copy link
Contributor

m-cat commented Nov 1, 2019

GitHub doesn't show a nice diff for the keys.rs file since it was renamed from public_key.rs. Reviewers might want to generate a diff locally.

Copy link
Member

lionel1704 left a comment

The changes look good. Seems like CI is failing since using Self:: in enums is experimental.

src/keys.rs Outdated Show resolved Hide resolved
@m-cat m-cat force-pushed the m-cat:secret-key branch 5 times, most recently from c5e23bf to 5ed38e8 Nov 4, 2019
@m-cat m-cat force-pushed the m-cat:secret-key branch 2 times, most recently from 20106b9 to 9e632d9 Nov 14, 2019
@m-cat m-cat changed the title feat/keys: add `SecretKey` enum similar to `PublicKey` feat/keys: add `Keypair` and remove bls methods for full IDs Nov 15, 2019
@m-cat m-cat force-pushed the m-cat:secret-key branch from 19c11c7 to bca9f6c Nov 20, 2019
Copy link
Member

lionel1704 left a comment

Looks good. Just one comment.

src/identity/mod.rs Outdated Show resolved Hide resolved
@m-cat m-cat force-pushed the m-cat:secret-key branch 2 times, most recently from 07f436f to a10c086 Nov 21, 2019
}

/// Returns the entity's public key, if applicable.
pub fn public_key(&self) -> Option<PublicKey> {

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Nov 21, 2019

Member

Can this function return just PublicKey now?
(Clippy has also failed)

This comment has been minimized.

Copy link
@m-cat

m-cat Nov 21, 2019

Author Contributor

The failures on Jenkins are due to an out of date container. However, I think @jacderida said that the container fails to build -- seems like it can't be updated without the lint allowance in this PR. I would say let's ignore Jenkins for this PR.

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Nov 21, 2019

Member

That's cool then! 👍

@m-cat m-cat force-pushed the m-cat:secret-key branch from a10c086 to 9655b28 Nov 21, 2019
Copy link
Member

lionel1704 left a comment

Looks good! Thanks! 👍

We've already done this in `safe_vault`, now trying it here. This change was
prompted by a false positive in the `clippy::type_repetition_in_bounds` lint.

See https://hackmd.io/8odS50_AQV6U1wCHBfbeuw for the motivation.

- Removed all `forbid` items: most are already `deny` by the compiler, not clear
what explicitly forbidding them gives us.

- `unsafe_code` stayed `deny`, other `deny` items removed as they are all `warn`
and will be caught by CI. The `bad_style` lint was deprecated in favor of
`nonstandard_style`, which is also `warn`.

- Existing `warn` items mostly unchanged, except that a couple were added. These
are all `allow` by the compiler, but useful to get warnings about.

- Removed all `allow` items: they are already `allow` by the compiler.
@m-cat m-cat force-pushed the m-cat:secret-key branch from 9655b28 to 58a3616 Nov 21, 2019
@lionel1704 lionel1704 merged commit b67c4ea into maidsafe:master Nov 21, 2019
4 of 5 checks passed
4 of 5 checks passed
Rustfmt-Clippy
Details
Test (ubuntu-latest)
Details
Test (windows-latest)
Details
Test (macOS-latest)
Details
Travis CI - Pull Request Build Canceled
Details
@m-cat m-cat deleted the m-cat:secret-key branch Nov 21, 2019
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.