Trying to understand store_sequence

Hello, I just started taking baby steps reading the code, beginning with sn_api, in particular, this function in sn_api/src/api/app/safe_client.rs puzzles me, so I wonder if someone could offer some hint?

pub async fn store_sequence(
    &mut self,
    data: &[u8],
    name: Option<XorName>,
    tag: u64,
    _permissions: Option<String>,
    private: bool,
) -> Result<XorName> {
    debug!(
        "Storing {} Sequence data with tag type: {:?}, xorname: {:?}",
        if private { "Private" } else { "Public" },
        tag,
        name
    );

    let mut safe_client = self.get_safe_client()?;
    let xorname = name.unwrap_or_else(rand::random);
    info!("Xorname for storage: {:?}", &xorname);

    let app_public_key = get_public_bls_key(&safe_client).await?;

    // The Sequence's owner will be the user
    let user_acc_owner = safe_client.public_key().await;

    // Store the Sequence on the network
    let address = if private {
        // Set permissions for append, delete, and manage perms to this application
        let mut perms = BTreeMap::default();
        let _ = perms.insert(
            SafeNdPublicKey::Bls(app_public_key),
            SequencePrivatePermissions::new(true, true, true),
        );

        safe_client
            .store_private_sequence(
                Some(vec![data.to_vec()]),
                xorname,
                tag,
                user_acc_owner,
                perms,
            )
            .await
            .map_err(|e| {
                Error::NetDataError(format!("Failed to store Private Sequence data: {:?}", e))
            })?
    } else {
        // Set permissions for append and manage perms to this application
        let user_app = SequenceUser::Key(SafeNdPublicKey::Bls(app_public_key));
        let mut perms = BTreeMap::default();
        let _ = perms.insert(user_app, SequencePublicPermissions::new(true, true));

        safe_client
            .store_public_sequence(
                Some(vec![data.to_vec()]),
                xorname,
                tag,
                user_acc_owner,
                perms,
            )
            .await
            .map_err(|e| {
                Error::NetDataError(format!("Failed to store Public Sequence data: {:?}", e))
            })?
    };

    let _op = safe_client
        .append_to_sequence(address, data.to_vec())
        .await
        .map_err(|e| {
            Error::NetDataError(format!("Failed to append data to the Sequence: {:?}", e))
        })?;

    Ok(xorname)
}

The question is, why does the sequence data have to be stored twice? Namely, at the end there’s this append_to_sequence call, but isn’t the data already stored before that?

My first guess was that, perhaps the initial “store” doesn’t actually store the data but rather creates a “placeholder”, and via its address, the subsequent “append” stores the actual data. But then when I looked into the store_private_sequence function, it did seem to add the actual data… so by far, I still can’t understand it.

So I thought, maybe I should ask a question? And I hope this is the right place to do so?

Thanks for your attention.

4 Likes

Hey @treslumen, thanks for digging about here! Totally fair question. The short answer is… this shouldn’t need to happen, you’ve found a wee bug there :+1:


Longer form explanation, that may be of interest as you’re looking at the code:

The sn_api lib has only just been updated to be compatible with the refactored sn_client code, which had quite a large amount of changes. As things stand sn_api compiles, but hasn’t been tested yet (we’re still squashing some last issues w/ e2e in sn_client and nodes re: data storage, once we have that in place we’ll be able to focus on sn_api properly).

So what you’re seeing here is a bug due to the refactor, previously what is now the store apis in sn_client, didn’t let you actually add anything to the sequence, so a call to append was needed directly after to do this.

After the refactor this is no longer the case, but I clearly missed removing this during the sn_api update.

I’ll make an issue to track this. Though if you’d like to PR in a fix, that would also be much appreciated.

Either way, thanks for taking a look and bringing this up. Super helpful :bow:


issue to track this: https://github.com/maidsafe/sn_api/issues/638

5 Likes

Oh I see, that makes sense, as I heard about the on-going refactoring. And thank you for taking the extra time to give the longer-form explanation. Although it’s a trivial bug, this once again shows that you guys not only care about the code, but the community who’s trying to learn about the technology, and this makes all the difference. I really appreciate such details.

Regarding the fix, of course I would like to help, because if I’m not mistaken, all you have to do is delete the append op at the end, right? But then the question is,would a PR just be pure overhead? It sounds like it’s fewer steps if you guys do the quick fix, right? Again, I’m asking because I don’t know exactly how you guys deal with such small things, so if indeed my creating a PR actually makes it easier for you guys, then do let me know, @joshuef.

4 Likes

Josh can answer, but it is good for the project even if you submit a simple PR as it shows community engagement and I think MaidSafe might like to mention it in a weekly update. Not many will see this here, so that’s good and may also encourage others. I’m really please to see this, so thanks from me too. :smile:

4 Likes

@treslumen, you’re very welcome :slight_smile:

Re: the PR. You are not mistaken. You’re right it could be sorted pretty easily by us. But as @happybeing notes, we’re keen for folk to get involved. So even if something is trivial, getting folk to PR can be good for a few reasons…

As well as a fix, (something we may not get to right away), it gets someone else more used to github and PRs (it seems like you’re well aware of this, but you never know!), so hopefully it would clarify that for a potential github newbie (or we would help them through the process; there’s also CI things to through to so there may be back and forth… all of which is helpful), and possibly down the line lead to some more PRs.

And yeh, it’s a nice thing to let folk know about in dev updates, it might inspire more folk to take a deeper look too :slight_smile: So aye, if you’re still keen, a PR would be much appreciated :+1:


Made a PR to add sn_fs info to the API docs so it’s clearer the current impl wil likely be deprecated, here: https://github.com/maidsafe/sn_api/pull/640

3 Likes

That’s merged now. Thanks very much @treslumen :bowing_man: :+1:

2 Likes

Thank you all for being so supportive of newcomers, however small a patch is; and it’s surely my pleasure to participate.

4 Likes