Skip to content

Commit

Permalink
refactor+style: Implement changes asked in PR reviews
Browse files Browse the repository at this point in the history 
* Simpify some code docs.
* Enhance `delete_buffered_single_signature` tests
* Make some naming more clear
  • Loading branch information
@Alenar
Alenar committed Sep 19, 2024
1 parent d06bb6e commit ad4d1db
Show file tree
Hide file tree
Showing 13 changed files with 101 additions and 81 deletions.
26 changes: 20 additions & 6 deletions ...gregator/src/database/query/buffered_single_signature/delete_buffered_single_signature.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ mod tests {
use mithril_persistence::sqlite::ConnectionExtensions;

use crate::database::query::GetBufferedSingleSignatureQuery;
use crate::database::record::strip_buffered_sigs_date;
use crate::database::test_helper::{insert_buffered_single_signatures, main_db_connection};

use super::*;
Expand All @@ -82,10 +83,17 @@ mod tests {
.unwrap();
assert_eq!(2, cursor.count());

let cursor = connection
.fetch(GetBufferedSingleSignatureQuery::all())
let remaining_records: Vec<BufferedSingleSignatureRecord> = connection
.fetch_collect(GetBufferedSingleSignatureQuery::all())
.unwrap();
assert_eq!(3, cursor.count());
assert_eq!(
strip_buffered_sigs_date(&BufferedSingleSignatureRecord::fakes(&[
("party_2", CardanoTransactions),
("party_1", CardanoTransactions),
("party_2", MithrilStakeDistribution),
])),
strip_buffered_sigs_date(&remaining_records)
);

let cursor = connection
.fetch(
Expand All @@ -97,9 +105,15 @@ mod tests {
.unwrap();
assert_eq!(2, cursor.count());

let cursor = connection
.fetch(GetBufferedSingleSignatureQuery::all())
let remaining_records: Vec<BufferedSingleSignatureRecord> = connection
.fetch_collect(GetBufferedSingleSignatureQuery::all())
.unwrap();
assert_eq!(1, cursor.count());
assert_eq!(
strip_buffered_sigs_date(&BufferedSingleSignatureRecord::fakes(&[(
"party_2",
MithrilStakeDistribution
),])),
strip_buffered_sigs_date(&remaining_records)
);
}
}
11 changes: 11 additions & 0 deletions mithril-aggregator/src/database/record/buffered_single_signature_record.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,17 @@ impl SqLiteEntity for BufferedSingleSignatureRecord {
}
}

/// Test only - strip the date from the given records to make them comparable.
#[cfg(test)]
pub(crate) fn strip_buffered_sigs_date(
records: &[BufferedSingleSignatureRecord],
) -> Vec<BufferedSingleSignatureRecord> {
records
.iter()
.map(BufferedSingleSignatureRecord::with_stripped_date)
.collect::<Vec<_>>()
}

#[cfg(test)]
mod tests {
use mithril_common::entities::SignedEntityTypeDiscriminants::{
Expand Down
25 changes: 9 additions & 16 deletions mithril-aggregator/src/database/repository/buffered_single_signature_repository.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,18 +107,11 @@ mod tests {
};
use mithril_common::test_utils::fake_keys;

use crate::database::record::BufferedSingleSignatureRecord;
use crate::database::record::{strip_buffered_sigs_date, BufferedSingleSignatureRecord};
use crate::database::test_helper::{insert_buffered_single_signatures, main_db_connection};

use super::*;

fn strip_date(records: &[BufferedSingleSignatureRecord]) -> Vec<BufferedSingleSignatureRecord> {
records
.iter()
.map(BufferedSingleSignatureRecord::with_stripped_date)
.collect::<Vec<_>>()
}

#[test]
fn retrieve_all() {
let connection = main_db_connection().unwrap();
Expand All @@ -136,12 +129,12 @@ mod tests {

let buffered_signatures_ctx = store.get_all().unwrap();
assert_eq!(
strip_date(&BufferedSingleSignatureRecord::fakes(&[
strip_buffered_sigs_date(&BufferedSingleSignatureRecord::fakes(&[
("party3", MithrilStakeDistribution),
("party2", CardanoTransactions),
("party1", CardanoTransactions),
])),
strip_date(&buffered_signatures_ctx)
strip_buffered_sigs_date(&buffered_signatures_ctx)
);
}

Expand All @@ -164,22 +157,22 @@ mod tests {
.get_by_discriminant::<BufferedSingleSignatureRecord>(CardanoTransactions)
.unwrap();
assert_eq!(
strip_date(&BufferedSingleSignatureRecord::fakes(&[
strip_buffered_sigs_date(&BufferedSingleSignatureRecord::fakes(&[
("party2", CardanoTransactions),
("party1", CardanoTransactions),
])),
strip_date(&buffered_signatures_ctx)
strip_buffered_sigs_date(&buffered_signatures_ctx)
);

let buffered_signatures_msd = store
.get_by_discriminant::<BufferedSingleSignatureRecord>(MithrilStakeDistribution)
.unwrap();
assert_eq!(
strip_date(&BufferedSingleSignatureRecord::fakes(&[(
strip_buffered_sigs_date(&BufferedSingleSignatureRecord::fakes(&[(
"party3",
MithrilStakeDistribution
),])),
strip_date(&buffered_signatures_msd)
strip_buffered_sigs_date(&buffered_signatures_msd)
);
}

Expand Down Expand Up @@ -295,11 +288,11 @@ mod tests {

let remaining_msd_sigs = store.get_all().unwrap();
assert_eq!(
strip_date(&BufferedSingleSignatureRecord::fakes(&[
strip_buffered_sigs_date(&BufferedSingleSignatureRecord::fakes(&[
("party4", CardanoTransactions),
("party2", MithrilStakeDistribution),
])),
strip_date(&remaining_msd_sigs)
strip_buffered_sigs_date(&remaining_msd_sigs)
);
}
}
16 changes: 8 additions & 8 deletions mithril-aggregator/src/http_server/routes/signatures_routes.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ mod handlers {
use crate::{
http_server::routes::reply,
message_adapters::FromRegisterSingleSignatureAdapter,
services::{CertifierService, CertifierServiceError, RegistrationStatus},
services::{CertifierService, CertifierServiceError, SignatureRegistrationStatus},
unwrap_to_internal_server_error, SingleSignatureAuthenticator,
};

Expand Down Expand Up @@ -99,8 +99,8 @@ mod handlers {
Ok(reply::server_error(err))
}
},
Ok(RegistrationStatus::Registered) => Ok(reply::empty(StatusCode::CREATED)),
Ok(RegistrationStatus::Buffered) => Ok(reply::empty(StatusCode::ACCEPTED)),
Ok(SignatureRegistrationStatus::Registered) => Ok(reply::empty(StatusCode::CREATED)),
Ok(SignatureRegistrationStatus::Buffered) => Ok(reply::empty(StatusCode::ACCEPTED)),
}
}
}
Expand All @@ -119,7 +119,7 @@ mod tests {
use crate::{
http_server::SERVER_BASE_PATH,
initialize_dependencies,
services::{CertifierServiceError, MockCertifierService, RegistrationStatus},
services::{CertifierServiceError, MockCertifierService, SignatureRegistrationStatus},
SingleSignatureAuthenticator,
};

Expand All @@ -145,7 +145,7 @@ mod tests {
.expect_register_single_signature()
.withf(|_, signature| signature.is_authenticated())
.once()
.return_once(move |_, _| Ok(RegistrationStatus::Registered));
.return_once(move |_, _| Ok(SignatureRegistrationStatus::Registered));
let mut dependency_manager = initialize_dependencies().await;
dependency_manager.certifier_service = Arc::new(mock_certifier_service);
dependency_manager.single_signer_authenticator =
Expand Down Expand Up @@ -210,7 +210,7 @@ mod tests {
let mut mock_certifier_service = MockCertifierService::new();
mock_certifier_service
.expect_register_single_signature()
.return_once(move |_, _| Ok(RegistrationStatus::Registered));
.return_once(move |_, _| Ok(SignatureRegistrationStatus::Registered));
let mut dependency_manager = initialize_dependencies().await;
dependency_manager.certifier_service = Arc::new(mock_certifier_service);

Expand Down Expand Up @@ -243,7 +243,7 @@ mod tests {
let mut mock_certifier_service = MockCertifierService::new();
mock_certifier_service
.expect_register_single_signature()
.return_once(move |_, _| Ok(RegistrationStatus::Buffered));
.return_once(move |_, _| Ok(SignatureRegistrationStatus::Buffered));
let mut dependency_manager = initialize_dependencies().await;
dependency_manager.certifier_service = Arc::new(mock_certifier_service);

Expand Down Expand Up @@ -276,7 +276,7 @@ mod tests {
let mut mock_certifier_service = MockCertifierService::new();
mock_certifier_service
.expect_register_single_signature()
.return_once(move |_, _| Ok(RegistrationStatus::Registered));
.return_once(move |_, _| Ok(SignatureRegistrationStatus::Registered));
let mut dependency_manager = initialize_dependencies().await;
dependency_manager.certifier_service = Arc::new(mock_certifier_service);

Expand Down
8 changes: 4 additions & 4 deletions mithril-aggregator/src/multi_signer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ pub trait MultiSigner: Sync + Send {
) -> StdResult<()>;

/// Verify a single signature using the stake distribution of the next epoch
async fn verify_single_signature_for_next_epoch(
async fn verify_single_signature_for_next_stake_distribution(
&self,
message: &str,
signatures: &entities::SingleSignatures,
Expand Down Expand Up @@ -87,7 +87,7 @@ impl MultiSigner for MultiSignerImpl {
self.run_verify_single_signature(message, single_signature, protocol_multi_signer)
}

async fn verify_single_signature_for_next_epoch(
async fn verify_single_signature_for_next_stake_distribution(
&self,
message: &str,
single_signature: &entities::SingleSignatures,
Expand Down Expand Up @@ -190,7 +190,7 @@ mod tests {
.await
.unwrap();

multi_signer.verify_single_signature_for_next_epoch(&message.to_message(), &signature).await.expect_err(
multi_signer.verify_single_signature_for_next_stake_distribution(&message.to_message(), &signature).await.expect_err(
"single signature issued in the current epoch should not be valid for the next epoch",
);
}
Expand All @@ -199,7 +199,7 @@ mod tests {
let next_epoch_signature = next_fixture.signers_fixture()[0].sign(&message).unwrap();

multi_signer
.verify_single_signature_for_next_epoch(
.verify_single_signature_for_next_stake_distribution(
&message.to_message(),
&next_epoch_signature,
)
Expand Down
44 changes: 21 additions & 23 deletions mithril-aggregator/src/services/certifier/buffered_certifier.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@ use mithril_common::StdResult;

use crate::entities::OpenMessage;
use crate::services::{
BufferedSingleSignatureStore, CertifierService, CertifierServiceError, RegistrationStatus,
BufferedSingleSignatureStore, CertifierService, CertifierServiceError,
SignatureRegistrationStatus,
};

/// A decorator of [CertifierService] that buffers that can buffer registration of single signatures
/// A decorator of [CertifierService] that can buffer registration of single signatures
/// when the open message is not yet created.
///
/// When an open message is created, buffered single signatures for the open message type are
Expand Down Expand Up @@ -90,7 +91,7 @@ impl CertifierService for BufferedCertifierService {
&self,
signed_entity_type: &SignedEntityType,
signature: &SingleSignatures,
) -> StdResult<RegistrationStatus> {
) -> StdResult<SignatureRegistrationStatus> {
match self
.certifier_service
.register_single_signature(signed_entity_type, signature)
Expand All @@ -110,7 +111,7 @@ impl CertifierService for BufferedCertifierService {
.buffer_signature(signed_entity_type.into(), signature)
.await?;

Ok(RegistrationStatus::Buffered)
Ok(SignatureRegistrationStatus::Buffered)
}
_ => Err(error),
},
Expand All @@ -122,14 +123,8 @@ impl CertifierService for BufferedCertifierService {
signed_entity_type: &SignedEntityType,
protocol_message: &ProtocolMessage,
) -> StdResult<OpenMessage> {
// IMPORTANT: this method should not fail if the open message creation succeeds.
// else:
// 1 - state machine won't create a pending certificate for the signed entity type
// 2 - Without a pending certificate, the signers won't send their signatures
// 3 - state machine will retry the transition to signing and, since an open message was
// opened for the signed entity type, it will try the next on the list.
// 4 - since the state machine never was in signing it will never try to aggregate
// signatures for the signed entity type
// IMPORTANT: this method should not fail if the open message creation succeeds
// Otherwise, the state machine won't aggregate signatures for this open message.

let creation_result = self
.certifier_service
Expand Down Expand Up @@ -234,7 +229,10 @@ mod tests {
async fn run_register_signature_scenario(
decorated_certifier_mock_config: impl FnOnce(&mut MockCertifierService),
signature_to_register: &SingleSignatures,
) -> (StdResult<RegistrationStatus>, Vec<SingleSignatures>) {
) -> (
StdResult<SignatureRegistrationStatus>,
Vec<SingleSignatures>,
) {
let store = Arc::new(BufferedSingleSignatureRepository::new(Arc::new(
main_db_connection().unwrap(),
)));
Expand Down Expand Up @@ -267,14 +265,14 @@ mod tests {
|mock_certifier| {
mock_certifier
.expect_register_single_signature()
.returning(|_, _| Ok(RegistrationStatus::Registered));
.returning(|_, _| Ok(SignatureRegistrationStatus::Registered));
},
&SingleSignatures::fake("party_1", "a message"),
)
.await;

let status = registration_result.expect("Registration should have succeed");
assert_eq!(status, RegistrationStatus::Registered);
assert_eq!(status, SignatureRegistrationStatus::Registered);
assert_eq!(
buffered_signatures_after_registration,
Vec::<SingleSignatures>::new()
Expand Down Expand Up @@ -306,7 +304,7 @@ mod tests {
.await;

let status = registration_result.expect("Registration should have succeed");
assert_eq!(status, RegistrationStatus::Buffered);
assert_eq!(status, SignatureRegistrationStatus::Buffered);
assert_eq!(
buffered_signatures_after_registration,
vec![SingleSignatures::fake("party_1", "a message")]
Expand Down Expand Up @@ -379,14 +377,14 @@ mod tests {
eq(SingleSignatures::fake("party_1", "message 1")),
)
.once()
.returning(|_, _| Ok(RegistrationStatus::Registered));
.returning(|_, _| Ok(SignatureRegistrationStatus::Registered));
mock.expect_register_single_signature()
.with(
eq(SignedEntityType::MithrilStakeDistribution(Epoch(5))),
eq(SingleSignatures::fake("party_2", "message 2")),
)
.once()
.returning(|_, _| Ok(RegistrationStatus::Registered));
.returning(|_, _| Ok(SignatureRegistrationStatus::Registered));
}),
store.clone(),
TestLogger::stdout(),
Expand Down Expand Up @@ -441,7 +439,7 @@ mod tests {

mock.expect_register_single_signature()
.with(always(), eq(fake_data::single_signatures(vec![1])))
.returning(|_, _| Ok(RegistrationStatus::Registered))
.returning(|_, _| Ok(SignatureRegistrationStatus::Registered))
.once();
mock.expect_register_single_signature()
.with(always(), eq(fake_data::single_signatures(vec![2])))
Expand All @@ -455,7 +453,7 @@ mod tests {
.once();
mock.expect_register_single_signature()
.with(always(), eq(fake_data::single_signatures(vec![3])))
.returning(|_, _| Ok(RegistrationStatus::Registered))
.returning(|_, _| Ok(SignatureRegistrationStatus::Registered))
.once();
},
|mock| {
Expand All @@ -482,7 +480,7 @@ mod tests {
mock.expect_create_open_message()
.returning(|_, _| Ok(OpenMessage::dummy()));
mock.expect_register_single_signature()
.returning(|_, _| Ok(RegistrationStatus::Registered));
.returning(|_, _| Ok(SignatureRegistrationStatus::Registered));
},
|mock| {
mock.expect_get_buffered_signatures()
Expand All @@ -493,7 +491,7 @@ mod tests {
}

#[tokio::test]
async fn do_not_return_an_error_if_getting_registering_signature_fail() {
async fn do_not_return_an_error_if_registering_signature_fail() {
run_scenario(
|mock| {
mock.expect_create_open_message()
Expand All @@ -516,7 +514,7 @@ mod tests {
mock.expect_create_open_message()
.returning(|_, _| Ok(OpenMessage::dummy()));
mock.expect_register_single_signature()
.returning(|_, _| Ok(RegistrationStatus::Registered));
.returning(|_, _| Ok(SignatureRegistrationStatus::Registered));
},
|mock| {
mock.expect_get_buffered_signatures()
Expand Down
Loading

0 comments on commit ad4d1db

Please sign in to comment.