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

Introduce ParentchainHandler on untrusted side #1064

Merged
merged 17 commits into from
Oct 14, 2022

Conversation

haerdib
Copy link
Contributor

@haerdib haerdib commented Oct 12, 2022

As a preparatory step for introducing multiple parentchains in the same worker:

  • Promoted the ParentchainBlockSyncer to a ParentchainHandler which handles the connection of the parentchain into the enclave. This should make it easier to handle multiple parentchains on the untrusted side, since it allows creating one ParentchainHandler struct per parentchain.
  • Merged SidechainApiMock implementations with EnclaveBaseMock

No new functionality introduced, refactoring only.

Closes #1048

@haerdib haerdib self-assigned this Oct 12, 2022
@haerdib haerdib changed the title Refactor: expand parentchain handler on untrusted side Refactor: introduce ParentchainHandler struct on untrusted side Oct 12, 2022
@haerdib haerdib added B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E0-breaksnothing A7-somethingelse labels Oct 13, 2022
@haerdib haerdib changed the title Refactor: introduce ParentchainHandler struct on untrusted side Introduce ParentchainHandler struct on untrusted side Oct 13, 2022
@haerdib haerdib changed the title Introduce ParentchainHandler struct on untrusted side Introduce ParentchainHandler on untrusted side Oct 13, 2022
@haerdib haerdib marked this pull request as ready for review October 13, 2022 11:15
service/src/main.rs Show resolved Hide resolved
Comment on lines -289 to +286
println!("IntegriTEE Worker v{}", VERSION);
println!("Integritee Worker v{}", VERSION);
Copy link
Contributor

Choose a reason for hiding this comment

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

This has been around for a long time 🤣 finally gets fixed now 🎉

Comment on lines +84 to +96
impl Sidechain for EnclaveMock {
fn sync_parentchain<ParentchainBlock: ParentchainBlockTrait>(
&self,
_blocks: &[sp_runtime::generic::SignedBlock<ParentchainBlock>],
_nonce: u32,
) -> EnclaveResult<()> {
Ok(())
}

fn execute_trusted_calls(&self) -> EnclaveResult<()> {
todo!()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a matter of contention: Ideally, your mocks only mock a single trait in order to keep them as specific but also as simple as possible. Whenever you only need the Sidechain trait mocked, you now have to use a mock (EnclaveMock) that implements more than you potentially want. And it is used by components that require other traits, which creates an indirect dependency.

If, however, all dependents of this trait always need that combination of traits, then it's necessary again (which I'm guessing is the case here?). If so, then we can leave it as is..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. I see. Let's leave as it is now, in light of that the Mock currently only used once and that the Traits themselves probably need a refactoring (parentchain_sync should not be Sidechain specific)

My follow-up question will be offline 😈

service/src/error.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@OverOrion OverOrion left a comment

Choose a reason for hiding this comment

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

LGTM!

@haerdib haerdib merged commit 3837075 into master Oct 14, 2022
@haerdib haerdib deleted the bh/modularize-light-client-init branch October 14, 2022 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A7-somethingelse B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E0-breaksnothing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend ParentchainBlockSyncer with init_light_client
3 participants