refactor: Re-arrange Chains traits for better composability

[ramizhasan111]

Pull Request

Closes: PRO-803

Checklist

Please conduct a thorough self-review before opening the PR.

• I am confident that the code works.
• I have updated documentation where appropriate.

Summary

This PR re-arranges the Chain traits so that we can have a subset of associated types of the chain trait that can be re-used for multiple chains. Specifically, the ChainCrypto types that are shared across chains with the same crypto scheme (EVM chains for example). We can have one implementation of ChainCrypto trait for EVM chains that can then be re-used for all EVM chains. This then enables us to use a single key and a single threshold signature pallet that coordinates the signing for that ChainCrypto that can be shared across multiple chains. This would mean that in CFE we would have a single key and multisig client for all EVM chains .

Moreover, this also merges the ChainAbi trait into the main Chain trait since it didn't make too much sense to have a separate trait for abi types since it was already needed to be implemented for all chains alongside the Chain trait.

So, instead of having:

trait Chain {..}
trait ChainCrypto: Chain {..}
trait ChainAbi: ChainCrypto {..}.

we have

trait Chain {
 	type ChainCrypto: ChainCrypto;
	...
}

[codecov]

Codecov Report

Merging #3912 (c7622a9) into main (c44279b) will decrease coverage by 0%.
The diff coverage is 56%.

@@          Coverage Diff          @@
##            main   #3912   +/-   ##
=====================================
- Coverage     71%     71%   -0%     
=====================================
  Files        366     370    +4     
  Lines      56798   56837   +39     
  Branches   56798   56837   +39     
=====================================
+ Hits       40545   40555   +10     
- Misses     14240   14252   +12     
- Partials    2013    2030   +17     

Files Changed	Coverage Δ
engine/multisig/src/crypto.rs	76% <ø> (ø)
engine/multisig/src/crypto/bitcoin.rs	91% <0%> (-1%)	⬇️
engine/multisig/src/crypto/ed25519.rs	86% <ø> (ø)
engine/multisig/src/crypto/key_id.rs	63% <ø> (ø)
engine/src/multisig.rs	0% <0%> (ø)
engine/src/p2p.rs	16% <0%> (ø)
.../state_chain_observer/sc_observer/crypto_compat.rs	0% <0%> (ø)
engine/src/witness/common.rs	0% <ø> (ø)
...nked_chain_source/chunked_by_vault/egress_items.rs	0% <ø> (ø)
engine/src/witness/eth/key_manager.rs	0% <0%> (ø)
... and 68 more

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[kylezs]

traits are fun 😄

[kylezs]

meta comment: given this is a refactor... it should be refactor: and probably something more specific than this too, like refactor: move ChainAbi inside Chain trait :)

[msgmaxim]

meta comment: given this is a refactor... it should be refactor: and probably something more specific than this too, like refactor: move ChainAbi inside Chain trait :)

I agree. Also, could we require (if we don't already) that each (non-trivial) PR should have a short summary with context/motivation? I try to make my PRs accessible to everyone, but I find it hard to keep up with changes coming from Berlin if they don't provide context.

[linear]

PRO-803 Rearrange Chain and ChainCrypto traits to allow many-to-one relationship

[dandanlen]

meta comment: given this is a refactor... it should be refactor: and probably something more specific than this too, like refactor: move ChainAbi inside Chain trait :)

I agree. Also, could we require (if we don't already) that each (non-trivial) PR should have a short summary with context/motivation? I try to make my PRs accessible to everyone, but I find it hard to keep up with changes coming from Berlin if they don't provide context.

+1

@ramizhasan111 the PR template is there for a reason ;)

I've added some Linear issues to track this, am aware there were none.

[ramizhasan111]

+1

@ramizhasan111 the PR template is there for a reason ;)

Sorry, thought about writing it but got a bit lazy haha
@msgmaxim i have added the summary now :)

[dandanlen]

Looking good 👍

[dandanlen]

Similar to the XFor<T, I> type aliases in the pallets, we could create:

type AggKeyFor<C> = <<C as Chain>::ChainCrypto as ChainCrypto>::AggKey; etc.

Might remove some of the noise from these declarations? For example this would become:
payload: &PayloadFor<C>,

[msgmaxim]

+1
@ramizhasan111 the PR template is there for a reason ;)

Sorry, thought about writing it but got a bit lazy haha @msgmaxim i have added the summary now :)

Thanks! 🙂

[dandanlen]

One minor suggestion, apart from that LGTM.

[dandanlen]

Please let's update from main and make sure there are no hidden conflicts.