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

Redirect older Client API to the new data type API #942

Merged
merged 2 commits into from Jul 31, 2019

Conversation

@lionel1704
Copy link
Member

commented Jul 31, 2019

Resolved #938

@lionel1704 lionel1704 requested a review from nbaksalyar as a code owner Jul 31, 2019

@lionel1704 lionel1704 requested review from m-cat and Yoga07 Jul 31, 2019

@@ -1429,6 +1429,12 @@ impl Vault {
_ => return Err(SndError::AccessDenied),
}
if self.contains_data(&data_name) {
// Published Immutable Data is de-duplicated
if let DataId::Immutable(addr) = data_name {
if let IDataAddress::Pub(_) = addr {

This comment has been minimized.

Copy link
@m-cat

m-cat Jul 31, 2019

Contributor

Can use addr.is_pub() from the latest safe-nd

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Jul 31, 2019

Author Member

Can we address this in a separate PR with the other breaking changes from safe-nd?

This comment has been minimized.

Copy link
@m-cat

m-cat Jul 31, 2019

Contributor

It's not a breaking change, just a small QoL improvement and it's already been merged. Would make the code here shorter, but it's up to you.

@nbaksalyar
Copy link
Member

left a comment

Great work, thank you! 👍
I've only one small question and a minor change request.

.map_err(From::from)
.into_box()
client
.get_seq_mdata_shell(access_container.name(), access_container.type_tag())

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Jul 31, 2019

Member

This can be replaced with

Suggested change
.get_seq_mdata_shell(access_container.name(), access_container.type_tag())
.get_mdata_version(access_container.name(), access_container.type_tag())
.map_err(From::from)
.into_box()
client
.get_seq_mdata_shell(access_container.name(), access_container.type_tag())

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Jul 31, 2019

Member

Same comment about get_mdata_version applies here.

let ownerss = vec![
btree_set![user /* , someone_else */], // currently can't handle having multiple owners
btree_set![someone_else],

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Jul 31, 2019

Member

I must've missed something, but I think this test should still fail with a randomly-generated PublicKey?
Also, it has 2 duplicate sets of user, so the test is needlessly redundant I believe.

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Jul 31, 2019

Author Member

I can't recall why this change was needed. Let me quickly check.

refactor/core: refactor older client API to use the new data type API
- refactor older API to redirect to the API of the new data types
- add necessary From and Into impls to convert the types
- fix logic / test failures due to changes such as deleting an entry
removes the entire entry etc.
- fix broken logic for published immutable data. errors should not be
returned when data is already existent

@lionel1704 lionel1704 force-pushed the lionel1704:redirect-old-api branch from b487785 to 9549fdb Jul 31, 2019

@lionel1704 lionel1704 force-pushed the lionel1704:redirect-old-api branch from 9549fdb to 568eef1 Jul 31, 2019

refactor/auth,app: refactor auth and app logic to use the new data types
- refactor internals of safe_auth to use the new data type logic
- add/remove permissions for an app to read data from the access
container when it is authenticated / revoked
- do not re-encrypt data in default containers when an application is revoked
- fix safe_app and safe_auth tests to match the logic of the new types

@lionel1704 lionel1704 force-pushed the lionel1704:redirect-old-api branch from 568eef1 to 7417b5d Jul 31, 2019

@lionel1704 lionel1704 requested review from nbaksalyar and m-cat Jul 31, 2019

@m-cat

m-cat approved these changes Jul 31, 2019

@m-cat

m-cat approved these changes Jul 31, 2019

@m-cat

m-cat approved these changes Jul 31, 2019

@nbaksalyar nbaksalyar merged commit f518938 into maidsafe:experimental Jul 31, 2019

1 of 3 checks passed

Travis CI - Pull Request Build Errored
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@lionel1704 lionel1704 deleted the lionel1704:redirect-old-api 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.