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

Deprecate the use of the older Immutable Data API #947

Merged
merged 4 commits into from Aug 9, 2019

Conversation

@lionel1704
Copy link
Member

commented Aug 2, 2019

Depends on maidsafe/safe-nd#94
Closes #961

@lionel1704 lionel1704 requested review from nbaksalyar, m-cat and Yoga07 Aug 2, 2019

@lionel1704 lionel1704 force-pushed the lionel1704:idata-fns branch from 120abbd to c9b45ab Aug 2, 2019

safe_app/src/tests/mod.rs Outdated Show resolved Hide resolved
safe_app/src/tests/mod.rs Outdated Show resolved Hide resolved

@lionel1704 lionel1704 force-pushed the lionel1704:idata-fns branch 2 times, most recently from 1fb4bd8 to 9091159 Aug 5, 2019

safe_core/src/immutable_data.rs Outdated Show resolved Hide resolved
safe_core/src/immutable_data.rs Outdated Show resolved Hide resolved

@lionel1704 lionel1704 force-pushed the lionel1704:idata-fns branch from a7cd21f to a5659a6 Aug 7, 2019

safe_core/src/client/mod.rs Outdated Show resolved Hide resolved
tests/src/lib.rs Outdated Show resolved Hide resolved
safe_core/src/nfs/writer.rs Outdated Show resolved Hide resolved

@lionel1704 lionel1704 force-pushed the lionel1704:idata-fns branch from a5659a6 to 5c08bc0 Aug 7, 2019

lionel1704 added some commits Aug 2, 2019

feat/idata: impl. storage of unpub. idata using the self encryptor
- Impl. generate address function for self encryption storage
- Enable tests that store data as unpublished immutable data via the
self encryptor
- Refactor NFS file helper functions to store / delete unpublished
immutable data as well

@lionel1704 lionel1704 force-pushed the lionel1704:idata-fns branch from 5c08bc0 to 21ee542 Aug 8, 2019

@lionel1704 lionel1704 requested a review from m-cat Aug 8, 2019

@@ -28,9 +28,10 @@ impl<C: Client> Reader<C> {
client: C,
storage: SelfEncryptionStorage<C>,
file: &File,
published: bool,

This comment has been minimized.

Copy link
@m-cat

m-cat Aug 8, 2019

Contributor

One lost spot where published parameter can be removed, making the API simpler :)

Overall the PR looks great, the code is very clean now!

refactor/nfs_file: Add published flag to the file and improve API
- Add the published flag to the file struct instead of passing it to all
the API. This would simplify the API and not require users to remember
if the file is published or not.

@lionel1704 lionel1704 force-pushed the lionel1704:idata-fns branch from 21ee542 to 5f8e129 Aug 8, 2019

@lionel1704 lionel1704 requested a review from m-cat Aug 8, 2019

safe_core/src/nfs/tests.rs Outdated Show resolved Hide resolved
@nbaksalyar
Copy link
Member

left a comment

Just one critical thing I noticed.

let data: IData = if published {
PubImmutableData::new(value).into()
} else {
UnpubImmutableData::new(value, unwrap!(client.public_bls_key()).into()).into()

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Aug 9, 2019

Member

I'd be wary of using unwrap! in the production code; this function call will panic if called on unregistered client. Why can't we return an error future instead?

In addition to that, I think it's better to add a new function in Client to return PublicKey rather than implicitly convert the BLS key specifically since we might want to change this in the future. So I would suggest just add this function to Clients:

pub fn public_key() -> Option<PublicKey> {
    self.public_bls_key().into()
}

and not unwrap! it but just return an error:

fry!(client.public_key().ok_or_else(|| CoreError::from("Attempted to use unregistered client to send mutation request")))
encryption_key: Option<shared_secretbox::Key>,
) -> Box<NfsFuture<DataMap>> {
immutable_data::get_value(client, name, encryption_key)
let kind = IDataKind::from_flag(published);
let address = IDataAddress::from_kind(kind, *name);

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Aug 9, 2019

Member

Maybe it'd be easier to just have this address as a function argument?
Just a suggestion though.

refactor/client: make the key fields in the client trait non-optional
- Previously the client_keys field for clients was Optional since
unregistered clients had no keys in SCL. With the new quic-p2p approach
a random key is generated for every unregistered session and assigned to
the key. Make all the key fields non-optional and change their getter
functions in the client trait accordingly
- refactor tests to use and_then and add comments to guide reader
- add address function to NFS file object

@lionel1704 lionel1704 requested review from m-cat and nbaksalyar Aug 9, 2019

@m-cat

m-cat approved these changes Aug 9, 2019

Copy link
Contributor

left a comment

Awesome! So many unwrap!s were removed 😄

These changes have been addressed. Other requested changes will come in a separate PR

@lionel1704 lionel1704 merged commit 7e315bf into maidsafe:experimental Aug 9, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@lionel1704 lionel1704 deleted the lionel1704:idata-fns branch Aug 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.