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

Implement methods needed for replication defined in EntryStore traits #102

Merged
merged 13 commits into from
May 17, 2022

Conversation

sandreae
Copy link
Member

@sandreae sandreae commented May 9, 2022

📋 Checklist

  • Add tests that cover your changes
  • Add this PR to the Unreleased section in CHANGELOG.md
  • Link this PR to any issues it closes
  • New files contain a SPDX license header

@sandreae sandreae mentioned this pull request May 9, 2022
8 tasks
Copy link
Collaborator

@pietgeursen pietgeursen left a comment

Choose a reason for hiding this comment

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

Hell yeah! Excited for this!

@@ -15,6 +15,20 @@ use p2panda_rs::storage_provider::traits::{AsStorageEntry, EntryStore};
use crate::db::models::entry::EntryRow;
use crate::db::provider::SqlStorage;

// Re-write once https://github.com/pietgeursen/lipmaa-link/pull/3 merged in lipma-link
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is merged and released in 0.2.1 🎉

Copy link
Collaborator

Choose a reason for hiding this comment

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

And also, yeah it's annoying the fact that technically lipmaa links can go all the way back to 0 but the bamboo spec starts link numbers at 1.

🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeh, I did a bit of a double take on that 😂

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I might add this to lipmaa link:

/// Get all the lipmaa link numbers back to some value
///
/// ```
/// use lipmaa_link::get_lipmaa_links_back_to;
///
/// let links = get_lipmaa_links_back_to(7, 1);
///
/// assert_eq!(links, vec![6, 5, 4, 1]);
///
/// ```
#[cfg(feature = "std")]
pub fn get_lipmaa_links_back_to(start: u64, lower_inclusive: u64) -> Vec<u64> {
    let mut path = Vec::new();

    let mut n = start;
    while n > lower_inclusive {
        n = lipmaa(n);
        path.push(n);
    }

    path
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok yeah, I think it's useful, published in 0.2.2

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice! Thanks, I'll use that for sure.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah, I remember some one-off bug we had some time ago 😅

@sandreae
Copy link
Member Author

Are you being blocked by this not being merged @pietgeursen ? Ideally I'd give it today for the others to review, but seeing as we're going into development anyway I think it would be ok to push ahead if that helps.

@pietgeursen
Copy link
Collaborator

pietgeursen commented May 10, 2022 via email

@adzialocha adzialocha linked an issue May 10, 2022 that may be closed by this pull request
@sandreae sandreae self-assigned this May 12, 2022
Comment on lines +22 to +25
pub struct StorageEntry {
entry_signed: EntrySigned,
operation_encoded: OperationEncoded,
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this previously because technically we don't need two structs (EntryRow and StorageEntry) to handle entries coming in and out of the db because the storage traits could be implemented directly on EntryRow. However, I decided to reintroduce it as I think it improves understanding of what each struct is for and we do need different structs in most other stores, so it matches and will be recognisable across the repo.

Copy link
Member

Choose a reason for hiding this comment

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

We need to document this!

Copy link
Member Author

Choose a reason for hiding this comment

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

Related issue here: p2panda/p2panda#320

Base automatically changed from restruct-modules to development May 17, 2022 10:05
@sandreae sandreae merged commit 4144d24 into development May 17, 2022
Copy link
Member

@cafca cafca left a comment

Choose a reason for hiding this comment

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

Branch was merged before review submitted so this is just half-way through

Comment on lines 3 to 4
pub mod entry;
pub mod log;
Copy link
Member

Choose a reason for hiding this comment

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

I quite liked the flattened export as long as these modules only contain one or a handful of exports.

WHERE
author = $1
AND log_id = $2
AND CAST(seq_num AS INTEGER) BETWEEN $3 and $4
Copy link
Member

Choose a reason for hiding this comment

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

While SQLite cannot store u64 it can work with them, but only if they are cast as NUMERIC I think.

SELECT CAST("18446744073709551610" as INTEGER) BETWEEN 18446744073709551609 AND 18446744073709551611; => 0
SELECT CAST("18446744073709551610" as NUMERIC) BETWEEN 18446744073709551609 AND 18446744073709551611; => 1

Copy link
Member

Choose a reason for hiding this comment

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

Related #122

async fn entry_at_seq_num(
/// Get an entry from storage by it's hash id.
///
/// Returns a result containing the entry wrapped in an option if it was
Copy link
Member

Choose a reason for hiding this comment

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

a result containing the entry wrapped in an option

I like having docstrings that explain the possible return values. I think writing out the full type like that is a bit verbose though.

@cafca cafca deleted the entry-store-updates branch May 17, 2022 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Entry store implementation
4 participants