Skip to content
This repository has been archived by the owner on Jun 25, 2021. It is now read-only.

fix/naming: fix up some names for consistency #80

Merged
merged 4 commits into from Jul 26, 2019

Conversation

mrcnski
Copy link
Contributor

@mrcnski mrcnski commented Jul 23, 2019

No description provided.

@mrcnski mrcnski requested a review from ustulation as a code owner July 23, 2019 08:28
Copy link
Contributor

@nbaksalyar nbaksalyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!
Just one thing needs to be changed.

@@ -714,7 +714,7 @@ impl Data {
}
}

pub fn owner(&self) -> PublicKey {
pub fn owners(&self) -> PublicKey {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should stay as owner because generally we shouldn't care if it's a multisig/BLS key or some other key.
It'd be better to change the .owners field name in SeqMutableData and UnseqMutableData.

Copy link
Contributor

@nbaksalyar nbaksalyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more minor thing I just noticed

/// Contains a set of owners of this data. DataManagers enforce that a mutation request is
/// coming from the MaidManager Authority of the Owner.
owners: PublicKey,
/// Contains the owner of this data. DataManagers enforce that a mutation request is coming from
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed that this comment mentions legacy names from Vaults.
Could you please change it to something like

/// Contains the public key of an owner or owners of this data.
/// Data Handlers in vaults enforce that a mutation request has a valid signature of the owner.

?

/// Contains a set of owners of this data. DataManagers enforce that a mutation request is
/// coming from the MaidManager Authority of the Owner.
owners: PublicKey,
/// Contains the owner of this data. DataManagers enforce that a mutation request is coming from
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here.

@nbaksalyar
Copy link
Contributor

Before we merge this, @m-cat could you please raise a PR for SAFE Client Libs to make it compatible with these changes? There are some places in Mock Vault where we use the .owners() name. SAFE Vault is confirmed to be working correctly with this PR integrated.

@mrcnski
Copy link
Contributor Author

mrcnski commented Jul 25, 2019

Sure, I am just waiting for the tiny-keccak compatibility fixes as I can't compile and test SCL right now, it seems.

@nbaksalyar nbaksalyar merged commit 514116e into maidsafe:master Jul 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants