Move justification code to primitives crate (#640)

* Move justification module to header-chain primitives crate * Get justification module compiling in new location * Get justification module tests compiling * Use justification code from `header-chain` crate Mostly compiles, having issues with std/test feature flags across crates. * Move some code around * Move justification tests to integration testing crate * Add `test-utils` crate * Remove tests and test-helper module from justification code * Use `test-utils` in Substrate bridge pallet tests * Remove `sp-keyring` related code from `pallet-substrate-bridge` * Remove `helpers` module from `pallet-substrate-bridge` * Add some documentation * Add more documentation * Fix typo Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
paritytech · Apr 10, 2024 · c6df992 · c6df992
1 parent 0280400
commit c6df992
Show file tree

Hide file tree

Showing 13 changed files with 338 additions and 240 deletions.
diff --git a/bridges/modules/substrate/Cargo.toml b/bridges/modules/substrate/Cargo.toml
@@ -15,6 +15,7 @@ serde = { version = "1.0", optional = true }
 
 # Bridge Dependencies
 
+bp-header-chain = { path = "../../primitives/header-chain", default-features = false }
 bp-runtime = { path = "../../primitives/runtime", default-features = false }
 
 # Substrate Dependencies
@@ -27,14 +28,15 @@ sp-std = { git = "https://github.com/paritytech/substrate.git", branch = "master
 sp-trie = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false }
 
 [dev-dependencies]
+bp-test-utils = {path = "../../primitives/test-utils" }
 sp-core = { git = "https://github.com/paritytech/substrate.git", branch = "master" }
 sp-io = { git = "https://github.com/paritytech/substrate.git", branch = "master" }
-sp-keyring = { git = "https://github.com/paritytech/substrate.git", branch = "master" }
 sp-state-machine = { git = "https://github.com/paritytech/substrate.git", branch = "master" }
 
 [features]
 default = ["std"]
 std = [
+	"bp-header-chain/std",
 	"bp-runtime/std",
 	"codec/std",
 	"finality-grandpa/std",

diff --git a/bridges/modules/substrate/src/fork_tests.rs b/bridges/modules/substrate/src/fork_tests.rs
@@ -54,11 +54,11 @@
 //! Import a finality proof for header 2 on fork 1. This finalty proof should fail to be imported
 //! because the header is an old header.
 
-use crate::justification::tests::*;
-use crate::mock::{helpers::*, *};
+use crate::mock::*;
 use crate::storage::{AuthoritySet, ImportedHeader};
 use crate::verifier::*;
 use crate::{BestFinalized, BestHeight, BridgeStorage, NextScheduledChange, PalletStorage};
+use bp_test_utils::{alice, authority_list, bob, make_justification_for_header};
 use codec::Encode;
 use frame_support::{IterableStorageMap, StorageValue};
 use sp_finality_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID};
@@ -456,8 +456,7 @@ where
 				let grandpa_round = 1;
 				let set_id = 1;
 				let authorities = authority_list();
-				let justification =
-					make_justification_for_header(&header, grandpa_round, set_id, &authorities).encode();
+				let justification = make_justification_for_header(header, grandpa_round, set_id, &authorities).encode();
 
 				let res = verifier
 					.import_finality_proof(header.hash(), justification.into())

diff --git a/bridges/modules/substrate/src/lib.rs b/bridges/modules/substrate/src/lib.rs
@@ -45,10 +45,8 @@ use sp_trie::StorageProof;
 // Re-export since the node uses these when configuring genesis
 pub use storage::{AuthoritySet, InitializationData, ScheduledChange};
 
-pub use justification::decode_justification_target;
 pub use storage_proof::StorageProofChecker;
 
-mod justification;
 mod storage;
 mod storage_proof;
 mod verifier;
@@ -622,8 +620,8 @@ impl<T: Config> BridgeStorage for PalletStorage<T> {
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use crate::mock::helpers::{authority_list, test_header, unfinalized_header};
-	use crate::mock::{run_test, Origin, TestRuntime};
+	use crate::mock::{run_test, test_header, unfinalized_header, Origin, TestRuntime};
+	use bp_test_utils::authority_list;
 	use frame_support::{assert_noop, assert_ok};
 	use sp_runtime::DispatchError;
 

diff --git a/bridges/modules/substrate/src/mock.rs b/bridges/modules/substrate/src/mock.rs
@@ -16,11 +16,11 @@
 
 //! Mock Runtime for Substrate Pallet Testing.
 //!
-//! Includes some useful testing utilities in the `helpers` module.
+//! Includes some useful testing types and functions.
 
 #![cfg(test)]
 
-use crate::Config;
+use crate::{BridgedBlockHash, BridgedBlockNumber, BridgedHeader, Config};
 use bp_runtime::Chain;
 use frame_support::{impl_outer_origin, parameter_types, weights::Weight};
 use sp_runtime::{
@@ -30,6 +30,9 @@ use sp_runtime::{
 };
 
 pub type AccountId = u64;
+pub type TestHeader = BridgedHeader<TestRuntime>;
+pub type TestNumber = BridgedBlockNumber<TestRuntime>;
+pub type TestHash = BridgedBlockHash<TestRuntime>;
 
 #[derive(Clone, Eq, PartialEq, Debug)]
 pub struct TestRuntime;
@@ -88,66 +91,16 @@ pub fn run_test<T>(test: impl FnOnce() -> T) -> T {
 	sp_io::TestExternalities::new(Default::default()).execute_with(test)
 }
 
-pub mod helpers {
-	use super::*;
-	use crate::storage::ImportedHeader;
-	use crate::{BridgedBlockHash, BridgedBlockNumber, BridgedHeader};
-	use finality_grandpa::voter_set::VoterSet;
-	use sp_finality_grandpa::{AuthorityId, AuthorityList};
-	use sp_keyring::Ed25519Keyring;
-
-	pub type TestHeader = BridgedHeader<TestRuntime>;
-	pub type TestNumber = BridgedBlockNumber<TestRuntime>;
-	pub type TestHash = BridgedBlockHash<TestRuntime>;
-	pub type HeaderId = (TestHash, TestNumber);
-
-	pub fn test_header(num: TestNumber) -> TestHeader {
-		let mut header = TestHeader::new_from_number(num);
-		header.parent_hash = if num == 0 {
-			Default::default()
-		} else {
-			test_header(num - 1).hash()
-		};
-
-		header
-	}
-
-	pub fn unfinalized_header(num: u64) -> ImportedHeader<TestHeader> {
-		ImportedHeader {
-			header: test_header(num),
-			requires_justification: false,
-			is_finalized: false,
-			signal_hash: None,
-		}
-	}
-
-	pub fn header_id(index: u8) -> HeaderId {
-		(test_header(index.into()).hash(), index as _)
-	}
-
-	pub fn extract_keyring(id: &AuthorityId) -> Ed25519Keyring {
-		let mut raw_public = [0; 32];
-		raw_public.copy_from_slice(id.as_ref());
-		Ed25519Keyring::from_raw_public(raw_public).unwrap()
-	}
-
-	pub fn voter_set() -> VoterSet<AuthorityId> {
-		VoterSet::new(authority_list()).unwrap()
-	}
-
-	pub fn authority_list() -> AuthorityList {
-		vec![(alice(), 1), (bob(), 1), (charlie(), 1)]
-	}
-
-	pub fn alice() -> AuthorityId {
-		Ed25519Keyring::Alice.public().into()
-	}
-
-	pub fn bob() -> AuthorityId {
-		Ed25519Keyring::Bob.public().into()
-	}
+pub fn test_header(num: TestNumber) -> TestHeader {
+	// We wrap the call to avoid explicit type annotations in our tests
+	bp_test_utils::test_header(num)
+}
 
-	pub fn charlie() -> AuthorityId {
-		Ed25519Keyring::Charlie.public().into()
+pub fn unfinalized_header(num: u64) -> crate::storage::ImportedHeader<TestHeader> {
+	crate::storage::ImportedHeader {
+		header: test_header(num),
+		requires_justification: false,
+		is_finalized: false,
+		signal_hash: None,
 	}
 }
diff --git a/bridges/modules/substrate/src/verifier.rs b/bridges/modules/substrate/src/verifier.rs
@@ -22,9 +22,9 @@
 //! has been signed off by the correct GRANDPA authorities, and also enact any authority set changes
 //! if required.
 
-use crate::justification::verify_justification;
 use crate::storage::{AuthoritySet, ImportedHeader, ScheduledChange};
 use crate::BridgeStorage;
+use bp_header_chain::justification::verify_justification;
 use finality_grandpa::voter_set::VoterSet;
 use sp_finality_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID};
 use sp_runtime::generic::OpaqueDigestItemId;
@@ -350,10 +350,9 @@ fn find_scheduled_change<H: HeaderT>(header: &H) -> Option<sp_finality_grandpa::
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use crate::justification::tests::*;
-	use crate::mock::helpers::*;
 	use crate::mock::*;
 	use crate::{BestFinalized, BestHeight, HeaderId, ImportedHeaders, PalletStorage};
+	use bp_test_utils::{alice, authority_list, bob, make_justification_for_header};
 	use codec::Encode;
 	use frame_support::{assert_err, assert_ok};
 	use frame_support::{StorageMap, StorageValue};

diff --git a/bridges/primitives/header-chain/Cargo.toml b/bridges/primitives/header-chain/Cargo.toml
@@ -6,13 +6,29 @@ authors = ["Parity Technologies <admin@parity.io>"]
 edition = "2018"
 license = "GPL-3.0-or-later WITH Classpath-exception-2.0"
 
-[dependencies.parity-scale-codec]
-version = "1.3.1"
-default-features = false
-features = ["derive"]
+[dependencies]
+codec = { package = "parity-scale-codec", version = "1.3.1", default-features = false }
+finality-grandpa = { version = "0.12.3", default-features = false }
+
+# Substrate Dependencies
+
+frame-support = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false }
+sp-core = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false }
+sp-finality-grandpa = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false }
+sp-runtime = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false }
+sp-std = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false }
+
+[dev-dependencies]
+bp-test-utils = { path = "../test-utils" }
+sp-keyring = { git = "https://github.com/paritytech/substrate.git", branch = "master" }
 
 [features]
 default = ["std"]
 std = [
-	"parity-scale-codec/std",
+	"codec/std",
+	"finality-grandpa/std",
+	"frame-support/std",
+	"sp-core/std",
+	"sp-finality-grandpa/std",
+	"sp-std/std",
 ]