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

Use workspace dependencies for tee-worker(s) #3097

Merged
merged 38 commits into from
Sep 27, 2024
Merged

Use workspace dependencies for tee-worker(s) #3097

merged 38 commits into from
Sep 27, 2024

Conversation

Kailai-Wang
Copy link
Collaborator

@Kailai-Wang Kailai-Wang commented Sep 26, 2024

Context

Warning

It's a breaking change for bitacross client due to the DirectRequestStatus type change

As topic.

This PR tries to unify the cargo dependencies of identity-worker and bitacross-worker to form a workspace-based configuration.

Please note:

  • the common crates are under tee-worker/common/
  • enclave-runtime needs to have its own workspace for sgx-specific patches
  • some crates aren't merged, as they are differently implemented and/or meant to be specific for each worker, e.g. sgx-runtime, stf-executor, enclave-api ... see e.g. identity/core and identity/core-primitives for those crates. It's still possible to merge them into a "common" crate, but bigger code change would be needed
  • as a result, the workspace would contain two crates with the same name (e.g. ita-stf), to overcome this, I prepended the crate name with the worker type, e.g. id-ita-stf vs bc-ita-stf.
  • enum DirectRequestStatus is defined with different entry orders in id- and bc-worker. I follow the one in id-worker to not break anything from IDHub. Do we need to change anything for bc-worker? cc @kziemianek
  • some changes can be seen as an attempt (example) to merge the crates, e.g. HandleParentchainEvents, FilterEvents which contains fn for both workers

fixes P-242
fixes P-552

@Kailai-Wang Kailai-Wang self-assigned this Sep 26, 2024
@Kailai-Wang
Copy link
Collaborator Author

fixes P-242
fixes P-552

Copy link

linear bot commented Sep 26, 2024

Copy link
Contributor

@jonalvarezz jonalvarezz left a comment

Choose a reason for hiding this comment

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

what a great effort, thanks @Kailai-Wang!

lgtm

@kziemianek
Copy link
Member

@Kailai-Wang this will be a breaking change for a client:
we need to:

  • update definition here
  • let relayer team know so they can adjust their code

Copy link
Contributor

@silva-fj silva-fj left a comment

Choose a reason for hiding this comment

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

It looks good overall 👍🏼
I can feel the pain of making this changes 😆

@Kailai-Wang
Copy link
Collaborator Author

Kailai-Wang commented Sep 27, 2024

@Kailai-Wang this will be a breaking change for a client: we need to:

  • update definition here
  • let relayer team know so they can adjust their code

Thanks @kziemianek !
Here it is: c96fc21
The only thing that I'm not that sure is if H256 type is natively supported - I assume yes, as I don't see errors when compiling and running it (bc-worker should not get TrustedOperationStatus anyway)

@Kailai-Wang Kailai-Wang enabled auto-merge (squash) September 27, 2024 09:15
@Kailai-Wang Kailai-Wang merged commit fe22fd9 into dev Sep 27, 2024
25 checks passed
@Kailai-Wang Kailai-Wang deleted the worker-ws-dep branch September 27, 2024 11:51
@kziemianek
Copy link
Member

@Kailai-Wang this will be a breaking change for a client: we need to:

  • update definition here
  • let relayer team know so they can adjust their code

Thanks @kziemianek ! Here it is: c96fc21 The only thing that I'm not that sure is if H256 type is natively supported - I assume yes, as I don't see errors when compiling and running it (bc-worker should not get TrustedOperationStatus anyway)

It should be supported see.

@Kailai-Wang Kailai-Wang added the C0-breaking Breaking change that will cause existing client to break label Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C0-breaking Breaking change that will cause existing client to break
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants