Get rid of store adapter in aggregator #2143

sfauvel · 2024-11-25T17:03:23Z

Content

Replace the store using the legacy store adapter and replace them with a repository implementation in the Aggregator.

This PR includes:

Removing of StoreAdapter from persistence
Remove links to StoreAdapter for signer_registration and CertificatePending in Aggregator
Remove usage of AdapterError

Note

If pruning is unit tested, there is no test that verify that it's really plugged and worked in production.
It's just a configuration in build_upkeep_service function in the builder.rs file that activate it.

Pre-submit checklist

Branch
- Tests are provided (if possible)
- Crates versions are updated (if relevant)
- CHANGELOG file is updated (if relevant)
- Commit sequence broadly makes sense
- Key commits have useful messages
PR
- No clippy warnings in the CI
- Self-reviewed the diff
- Useful pull request description
- Reviewer requested
Documentation
- Update README file (if relevant)
- Update documentation website (if relevant)
- Add dev blog post (if relevant)

Comments

Issue(s)

Relates to #2118

…le and use `projection_with_table`

…ingStore` that use the `StoreAdapter`

mithril-aggregator/src/http_server/routes/signer_routes.rs

github-actions · 2024-11-25T17:11:27Z

Test Results

4 files ± 0 51 suites ±0 11m 54s ⏱️ +10s
1 410 tests - 45 1 410 ✅ - 45 0 💤 ±0 0 ❌ ±0
1 621 runs - 45 1 621 ✅ - 45 0 💤 ±0 0 ❌ ±0

Results for commit e450847. ± Comparison against base commit 67d7501.

This pull request removes 61 and adds 16 tests. Note that renamed tests count towards both.

mithril-aggregator ‑ database::repository::signer_registration_store::tests::test_signer_registration_store::can_prune_keys_from_given_epoch_retention_limit
mithril-aggregator ‑ database::repository::signer_registration_store::tests::test_signer_registration_store::get_signers_for_empty_epoch
mithril-aggregator ‑ database::repository::signer_registration_store::tests::test_signer_registration_store::get_signers_for_existing_epoch
mithril-aggregator ‑ database::repository::signer_registration_store::tests::test_signer_registration_store::get_verification_keys_for_empty_epoch
mithril-aggregator ‑ database::repository::signer_registration_store::tests::test_signer_registration_store::get_verification_keys_for_existing_epoch
mithril-aggregator ‑ database::repository::signer_registration_store::tests::test_signer_registration_store::save_key_in_empty_store
mithril-aggregator ‑ database::repository::signer_registration_store::tests::test_signer_registration_store::update_signer_in_store
mithril-aggregator ‑ signer_registerer::tests::should_prune_verification_keys_older_than_two_epochs_at_round_opening
mithril-aggregator ‑ store::pending_certificate_store::test::get_certificate_pending_with_existing_certificate
mithril-aggregator ‑ store::pending_certificate_store::test::get_certificate_pending_with_no_existing_certificate
…

mithril-aggregator ‑ database::repository::pending_certificate_repository::test::get_certificate_pending_with_existing_certificate
mithril-aggregator ‑ database::repository::pending_certificate_repository::test::get_certificate_pending_with_no_existing_certificate
mithril-aggregator ‑ database::repository::pending_certificate_repository::test::remove_certificate_pending
mithril-aggregator ‑ database::repository::pending_certificate_repository::test::save_certificate_pending_once
mithril-aggregator ‑ database::repository::pending_certificate_repository::test::should_migrate_data_from_adapter
mithril-aggregator ‑ database::repository::pending_certificate_repository::test::update_certificate_pending
mithril-aggregator ‑ database::repository::signer_registration_store::tests::can_prune_keys_from_given_epoch_retention_limit
mithril-aggregator ‑ database::repository::signer_registration_store::tests::get_signers_for_empty_epoch
mithril-aggregator ‑ database::repository::signer_registration_store::tests::get_signers_for_existing_epoch
mithril-aggregator ‑ database::repository::signer_registration_store::tests::get_verification_keys_for_empty_epoch
…

♻️ This comment has been updated with latest results.

sfauvel · 2024-11-26T10:08:18Z

mithril-aggregator/src/database/record/certificate_pending.rs

+
+impl CertificatePendingRecord {
+    /// Construct a [Projection] that will allow to hydrate this `CertificatePendingRecord`.
+    pub fn get_projection_with_table(table: &str) -> Projection {


It's a proposition to have a function that produce projection with that final table name instead of using an alias.
Tell me if we put it in this PR or if you prefer to keep the standard way

The alias mechanism is not really necessary because we always calling it just after creating the projection. There is no reuse of the projection.
With this function, the caller knows which table name(s) need to be map instead of using an alias key string without any constraints and used in different files (we may use a constant).
Callers of projection function are requests that know record type and can use specific function of this record.

sfauvel · 2024-11-26T10:10:48Z

mithril-aggregator/src/database/query/pending_certificate/save_pending_certificate.rs

+        // it is important to alias the fields with the same name as the table
+        // since the table cannot be aliased in a RETURNING statement in SQLite.
+
+        // let projection = Self::Entity::get_projection().expand(SourceAlias::new(&[(


This comment will be remove if we keep get_projection_with_table otherwise it will be uncommented and the code below will be removed.
See PR comment on get_projection_with_table in CertificatePendingRecord to have explanation about this function.

sfauvel · 2024-11-26T10:15:58Z

mithril-aggregator/src/dependency_injection/builder.rs

+        &mut self,
+    ) -> Result<Arc<dyn CertificatePendingStorer>> {
+        Ok(Arc::new(CertificatePendingRepository::new(
+            self.get_sqlite_connection().await?,


The building of certificate_pending_store is know the same in production and in test environment.
There is no really need to make a difference.

OK, it's better like this 👍

mithril-aggregator/src/dependency_injection/builder.rs

sfauvel · 2024-11-26T10:43:47Z

internal/mithril-persistence/src/store/mod.rs

 mod stake_store;
-mod store_pruner;

 pub use stake_store::StakeStorer;


We keep StakeStore that contain the trait.
Maybe it's could be better to move this trait from persistence to aggregator and signer
What do you think ?

jpraynaud

Overall looks good 👍

I left few questions and suggestions.

jpraynaud · 2024-11-26T14:17:13Z

mithril-aggregator/src/database/query/pending_certificate/get_pending_certificate.rs

+    }
+
+    fn get_definition(&self, condition: &str) -> String {
+        // let aliases = SourceAlias::new(&[("{:pending_certificate:}", "new_pending_certificate")]);


You forgot to remove this comment

I rework it using the new function expand_projection

jpraynaud · 2024-11-26T14:17:30Z

mithril-aggregator/src/database/query/pending_certificate/get_pending_certificate.rs

+        let projection = Self::Entity::get_projection_with_table("pending_certificate")
+            .expand(SourceAlias::new(&[]));
+        format!(
+            // TODO check the order to keep


Let's keep this order (rowid desc). You can remove this comment.

jpraynaud · 2024-11-26T14:18:01Z

mithril-aggregator/src/database/query/pending_certificate/save_pending_certificate.rs

+
+use crate::database::record::CertificatePendingRecord;
+
+/// Query to update [CertificatePendingRecord] in the sqlite database


Suggested change

/// Query to update [CertificatePendingRecord] in the sqlite database

/// Query to save [CertificatePendingRecord] in the sqlite database

jpraynaud · 2024-11-26T14:20:59Z

mithril-aggregator/src/database/migration.rs

+            r#"
+create table new_pending_certificate (
+    epoch                           integer     not null,
+    certificate                     text        not null,


certificate is a bit misleading here as this does not represent a certificate.
But let's keep it like that if it requires too many modifications as we will remove it completely in the near future.

If you have a better name, we can change it. It's not a lot of work.

jpraynaud · 2024-11-26T14:24:26Z

mithril-aggregator/src/database/record/certificate_pending.rs

+    }
+}
+
+impl From<CertificatePending> for CertificatePendingRecord {


It'd be better to implement TryFrom here to avoid panics.

jpraynaud · 2024-11-26T14:49:50Z

mithril-aggregator/src/database/repository/signer_registration_store.rs


    use super::*;

+    fn build_signers(


You could have used a helper to create a SignerWithStake from a party_id and a verification_key.
Also the code could probably be a bit more functional.

jpraynaud · 2024-11-26T14:58:29Z

mithril-aggregator/src/database/test_helper.rs

@@ -456,3 +458,52 @@ pub fn insert_stake_pool(

    Ok(())
 }
+
+/// A simple struct that help to initialize database with the old adapter behavior for testing purposes.


Suggested change

/// A simple struct that help to initialize database with the old adapter behavior for testing purposes.

/// A simple struct that helps initialize a database with the old adapter behavior for testing purposes.

jpraynaud · 2024-11-26T14:58:56Z

mithril-aggregator/src/database/test_helper.rs

+        self.connection.execute(sql).unwrap();
+    }
+
+    pub fn is_key_hash_exist(&self, key_hash: &str) -> bool {


Suggested change

pub fn is_key_hash_exist(&self, key_hash: &str) -> bool {

pub fn ihas_key_hash(&self, key_hash: &str) -> bool {

jpraynaud · 2024-11-26T15:00:00Z

mithril-aggregator/src/dependency_injection/builder.rs

+        &mut self,
+    ) -> Result<Arc<dyn CertificatePendingStorer>> {
+        Ok(Arc::new(CertificatePendingRepository::new(
+            self.get_sqlite_connection().await?,


OK, it's better like this 👍

jpraynaud · 2024-11-26T15:02:05Z

mithril-aggregator/src/services/epoch_service.rs

-                .unwrap(),
-            ));
+
+            let vkey_store = {


Suggested change

let vkey_store = {

let verification_key_store = {

dlachaume

LGTM 👍

dlachaume · 2024-11-26T16:03:11Z

mithril-aggregator/src/database/repository/pending_certificate_repository.rs

+                migrations
+                    .clone()
+                    .into_iter()
+                    .filter(|m| m.version < 32)


Just a reminder if you want to update the version number after rebasing on the main branch.

dlachaume · 2024-11-26T16:14:19Z

mithril-aggregator/src/dependency_injection/builder.rs

-    /// Get a configured [CertificatePendingStore].
-    pub async fn get_certificate_pending_store(&mut self) -> Result<Arc<CertificatePendingStore>> {
+    /// Get a configured [CertificatePendingStorer].
+    pub async fn get_certificate_pending_store(


Should we rename the function as well?

Suggested change

pub async fn get_certificate_pending_store(

pub async fn get_certificate_pending_storer(

I also rename build_certificate_pending_store to build_certificate_pending_storer

sfauvel added 5 commits November 21, 2024 12:35

Create a PendingCertificateRepository

1676221

Rename column content to certificate in pending_certificate tab…

2ab3cb7

…le and use `projection_with_table`

Create a CertificatePendingStorer trait

7c523bf

Create a trait CertificatePendingStorer and remove `CertificatePend…

19df65f

…ingStore` that use the `StoreAdapter`

Replace VerificationKeyStore by mock or real implementation in tests

b4ec1a6

github-advanced-security bot found potential problems Nov 25, 2024

View reviewed changes

mithril-aggregator/src/http_server/routes/signer_routes.rs Fixed Show fixed Hide fixed

sfauvel force-pushed the sfa/2118/get_rid_of_store_adapter_in_aggregator branch from 7363c3c to dae2e96 Compare November 26, 2024 09:41

sfauvel commented Nov 26, 2024

View reviewed changes

sfauvel marked this pull request as ready for review November 26, 2024 10:23

sfauvel requested review from jpraynaud, Alenar and dlachaume November 26, 2024 10:23

sfauvel force-pushed the sfa/2118/get_rid_of_store_adapter_in_aggregator branch from dae2e96 to d0ba1ee Compare November 26, 2024 10:26

github-advanced-security bot found potential problems Nov 26, 2024

View reviewed changes

mithril-aggregator/src/dependency_injection/builder.rs Fixed Show fixed Hide fixed

sfauvel commented Nov 26, 2024

View reviewed changes

sfauvel force-pushed the sfa/2118/get_rid_of_store_adapter_in_aggregator branch from d0ba1ee to 20c0332 Compare November 26, 2024 10:45

sfauvel added 2 commits November 26, 2024 14:13

Migrate pruning of signer registrations in the upkeep service

82e196a

Remove StoreAdapter

e450847

sfauvel force-pushed the sfa/2118/get_rid_of_store_adapter_in_aggregator branch from 20c0332 to e450847 Compare November 26, 2024 13:13

sfauvel temporarily deployed to testing-preview November 26, 2024 13:33 — with GitHub Actions Inactive

sfauvel temporarily deployed to testing-sanchonet November 26, 2024 13:33 — with GitHub Actions Inactive

jpraynaud reviewed Nov 26, 2024

View reviewed changes

dlachaume approved these changes Nov 26, 2024

View reviewed changes

Alenar approved these changes Nov 26, 2024

View reviewed changes

Replace get_projection_with_table by expand_projection

61fd2ee

sfauvel force-pushed the sfa/2118/get_rid_of_store_adapter_in_aggregator branch from d78085f to 61fd2ee Compare November 26, 2024 17:53

sfauvel added 2 commits November 26, 2024 19:03

Replace Fromby TryFrom to avoid panics

fe53f31

Some renaming

b974e02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of store adapter in aggregator #2143

Get rid of store adapter in aggregator #2143

sfauvel commented Nov 25, 2024 •

edited

Loading

github-actions bot commented Nov 25, 2024 •

edited

Loading

sfauvel Nov 26, 2024

sfauvel Nov 26, 2024

sfauvel Nov 26, 2024

jpraynaud Nov 26, 2024

sfauvel Nov 26, 2024

jpraynaud left a comment

jpraynaud Nov 26, 2024

sfauvel Nov 26, 2024

jpraynaud Nov 26, 2024

jpraynaud Nov 26, 2024

jpraynaud Nov 26, 2024

sfauvel Nov 26, 2024

jpraynaud Nov 26, 2024

jpraynaud Nov 26, 2024

jpraynaud Nov 26, 2024

jpraynaud Nov 26, 2024

jpraynaud Nov 26, 2024

jpraynaud Nov 26, 2024

dlachaume left a comment

dlachaume Nov 26, 2024

dlachaume Nov 26, 2024

sfauvel Nov 26, 2024


		use crate::database::record::CertificatePendingRecord;

		/// Query to update [CertificatePendingRecord] in the sqlite database

	/// Query to update [CertificatePendingRecord] in the sqlite database
	/// Query to save [CertificatePendingRecord] in the sqlite database

	/// A simple struct that help to initialize database with the old adapter behavior for testing purposes.
	/// A simple struct that helps initialize a database with the old adapter behavior for testing purposes.

	pub fn is_key_hash_exist(&self, key_hash: &str) -> bool {
	pub fn ihas_key_hash(&self, key_hash: &str) -> bool {

	pub async fn get_certificate_pending_store(
	pub async fn get_certificate_pending_storer(

Get rid of store adapter in aggregator #2143

Are you sure you want to change the base?

Get rid of store adapter in aggregator #2143

Conversation

sfauvel commented Nov 25, 2024 • edited Loading

Content

Pre-submit checklist

Comments

Issue(s)

github-actions bot commented Nov 25, 2024 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpraynaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlachaume left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfauvel commented Nov 25, 2024 •

edited

Loading

github-actions bot commented Nov 25, 2024 •

edited

Loading