WIP: implement public appendable data #1133

Closed
wants to merge 14 commits into
from

Projects

None yet

6 participants

@afck
Member
afck commented Aug 31, 2016 edited

This change is Reviewable

@maidsafe-highfive

@afck: no appropriate reviewer found, use r? to override

@afck afck WIP: implement public appendable data
1b63980
@maqi
Member
maqi commented Sep 1, 2016

src/pub_appendable_data.rs, line 125 [r1] (raw file):

    /// The `data` will contain the union of the data items, _excluding_ the `deleted_data` as
    /// given in the update.
    pub fn update_with_other(&mut self, other: PubAppendableData) -> Result<(), RoutingError> {

as this is appendable data, and appending data will update version automatically, is this update function really needed?


Comments from Reviewable

@maqi
Member
maqi commented Sep 1, 2016

Review status: 0 of 18 files reviewed at latest revision, 5 unresolved discussions.


src/lib.rs, line 202 [r1] (raw file):

pub use node::{Node, NodeBuilder};
pub use plain_data::PlainData;
pub use pub_appendable_data::{MAX_PUB_APPENDABLE_DATA_SIZE_IN_BYTES, PubAppendableData,

seems this max_size hasn't been used for any self check?


src/pub_appendable_data.rs, line 245 [r1] (raw file):

    /// the number of signatures that are still required. If more than 50% of the previous owners
    /// have signed, 0 is returned and validation is complete.
    pub fn add_signature(&mut self, secret_key: &SecretKey) -> Result<usize, RoutingError> {

do we need to ensure the secret_key used to adding signature is from allowed participant?


src/pub_appendable_data.rs, line 258 [r1] (raw file):

    /// Overwrite any existing signatures with the new signatures provided.
    pub fn replace_signatures(&mut self, new_signatures: Vec<Signature>) {

no need to carry out any check here?


src/pub_appendable_data.rs, line 263 [r1] (raw file):

    /// Get the data
    pub fn get_data(&self) -> &BTreeSet<AppendedData> {

not sure, but as I remember get_ is preferred not to be used?


Comments from Reviewable

@afck
Member
afck commented Sep 2, 2016

Review status: 0 of 18 files reviewed at latest revision, 5 unresolved discussions.


src/pub_appendable_data.rs, line 263 [r1] (raw file):

Previously, maqi wrote…

not sure, but as I remember get_ is preferred not to be used?

I agree, but most of this is taken over from structured data, and there it's called `get_data`. I'd leave it like this for consistency for now, and make the change for all data types in a separate PR.

Comments from Reviewable

maqi and others added some commits Sep 5, 2016
@maqi maqi feat/priv_appendable_data 8a90bc5
@afck afck feat/tests: randomise message handling order in mock crust 6ee3c39
@afck afck refactor AppendWrapper
5ba760f
@maqi maqi Merge branch '0.23' of github.com:afck/routing into 0.23 47ce12d
@maqi maqi feat/append_types: AppendWrapper ce5da4d
@maqi maqi feat/check against filter when appending data ff10dab
@maqi maqi feat/serialised_priv_appended_data_size a223792
@maqi maqi chore/rustfmt corrections 8d6ec5e
@afck afck Merge pull request #15 from maqi/0.23
feat/priv_appendable_data
36d5368
@afck afck fix/appendable_data: make fields pub and fix Clippy
106e6ff
@afck afck Merge remote-tracking branch 'upstream/0.23' into 0.23
f18dee8
@afck afck fix/priv_appendable_data: fix appended data encryption
8463c1c
@afck afck feat/appendable_data: remove payload_size, implement signatures
901e8aa
@ustulation ustulation commented on the diff Sep 7, 2016
src/priv_appendable_data.rs
+ /// The filter defining who is allowed to append items.
+ pub filter: Filter,
+ /// The key to use for encrypting appended data items.
+ pub encrypt_key: box_::PublicKey,
+ /// A collection of previously deleted data items.
+ pub deleted_data: BTreeSet<PrivAppendedData>,
+ /// The signatures of the above fields by the previous owners, confirming the last update.
+ pub previous_owner_signatures: Vec<Signature>, // All the above fields
+ /// The collection of appended data items. These are not signed by the owners, as they change
+ /// even between `Post`s.
+ pub data: BTreeSet<PrivAppendedData>, // Unsigned
+}
+
+impl PrivAppendableData {
+ /// Creates a new `PubAppendableData` signed with `signing_key`.
+ pub fn new(name: XorName,
@ustulation
ustulation Sep 7, 2016 Member

Shouldn't this function also take delete_data parameter instead of initialising with a null (empty set) ? It is a signed field so i can't add it later without invalidating the signature. So if i fetch Pub/PrivAppendableData and want to change a field i will need to create a new one which is the clone of the previous one with changes in version (to re-post) and that field of choice. This will also mean cloning of deleted-data and will need to pass that to function parameter here.

@Fraser999
Fraser999 Sep 8, 2016 Member

Can't you just clone the data and mutate the clone? You don't need to go via new() do you?

@ustulation
ustulation Sep 8, 2016 Member

new() already signs stuff and deleted-data is included in signature. So it should be exposed as a parameter in new().Adding it later will invalidate signature. It's just like data field of structured data. You take that as a parameter in new(). For the same reasons.

@Fraser999
Fraser999 Sep 8, 2016 Member

Well, as we discussed briefly yesterday, I'd prefer a different API to this altogether (all members private, with mutating functions which would do all that is required to the member fields), but yeah - it probably does make sense to add it for now.

@ustulation ustulation commented on the diff Sep 7, 2016
src/lib.rs
@@ -198,6 +201,9 @@ pub use immutable_data::ImmutableData;
pub use messages::{Request, Response};
pub use node::{Node, NodeBuilder};
pub use plain_data::PlainData;
+pub use append_types::{AppendedData, AppendWrapper, Filter};
+pub use pub_appendable_data::{MAX_PUB_APPENDABLE_DATA_SIZE_IN_BYTES, PubAppendableData};
+pub use priv_appendable_data::{MAX_PRIV_APPENDABLE_DATA_SIZE_IN_BYTES, PrivAppendableData};
@ustulation
ustulation Sep 7, 2016 Member

PrivAppendedData is not exposed - needs to be exposed too.

@maqi
Member
maqi commented Sep 8, 2016

src/priv_appendable_data.rs, line 101 [r5] (raw file):

Previously, Fraser999 (Fraser Hutchison) wrote…

Well, as we discussed briefly yesterday, I'd prefer a different API to this altogether (all members private, with mutating functions which would do all that is required to the member fields), but yeah - it probably does make sense to add it for now.

does it mean inside delete function, actually we needs to update signature as well?

Comments from Reviewable

@Viv-Rajkumar
Member

src/priv_appendable_data.rs, line 101 [r5] (raw file):

Previously, maqi wrote…

does it mean inside delete function, actually we needs to update signature as well?

good point @maqi . I guess we have to since we update the `deleted_data` field there. This fn is only used client side right?

Comments from Reviewable

@Fraser999
Member

Review status: 0 of 14 files reviewed at latest revision, 6 unresolved discussions.


src/priv_appendable_data.rs, line 101 [r5] (raw file):

Previously, Viv-Rajkumar (Viv Rajkumar) wrote…

good point @maqi . I guess we have to since we update the deleted_data field there. This fn is only used client side right?

Not sure if the intent here though is to be able to call `delete()` multiple times before finally incrementing the version and signing everything.

Comments from Reviewable

@maqi
Member
maqi commented Sep 8, 2016

src/priv_appendable_data.rs, line 101 [r5] (raw file):

Previously, Fraser999 (Fraser Hutchison) wrote…

Not sure if the intent here though is to be able to call delete() multiple times before finally incrementing the version and signing everything.

i guess the intent is to call delete() some times then call add_signature to sign everything. however, as such usage is not enforced (means chance of misuse), maybe we shall change it it delete(datas: vec, sign_key: SecretKey) to enforce the usage pattern?i think same shall be applied to append.

Comments from Reviewable

@Viv-Rajkumar
Member

Closing this as its included with #1136

@afck afck deleted the afck:0.23 branch Dec 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment