-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: shared quartz_common crate + enclave server macro #87
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments. Also when I tried to run in MOCK_SGX mode for the transfer app, I got this error:
QUARTZ_PORT is set to: 11090
set trusted hash
... 09E63DFD91ECD4394ABF3D7222AAF60D49253549C19B650B31DCFF722C2B934F
MOCK_SGX is set. Running enclave without gramine...
Error: Os { code: 2, kind: NotFound, message: "No such file or directory" }
Did you also get this error? If you don't have it, then it must be my environment. But if you have it as well, it might be an error related to one of my comments below, or something else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I did preliminary review and added some comments.
Nit: Cargo.tomls are not formatted and deps are not alphabetically ordered 😅. (feel free to ignore)
Haven't tested yet - just want to make sure mock-sgx is working correctly but will wait for the fix for using DefaultAttestor
in the enclaves. Will try to test tonight otherwise will delegate to the team (because I'm off for the rest of the week). 🙏
@@ -0,0 +1,64 @@ | |||
#[cfg(feature = "macro")] | |||
#[macro_export] | |||
macro_rules! quartz_server { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the macro is cool, but I am not convinced we need it for the following reasons -
- The main reason to have this macro IMHO would be to make sure the user/dev didn't forget to add the quartz
CoreService
. But I think there are other ways to achieve that. e.g. ->
trait QuartzServerExt {
fn quartz_builder(
config: Config,
sk: Arc<Mutex<Option<SigningKey>>>,
attestor: DefaultAttestor,
) -> Router;
}
impl QuartzServerExt for Server {
fn quartz_builder(
config: Config,
sk: Arc<Mutex<Option<SigningKey>>>,
attestor: DefaultAttestor,
) -> Router {
Server::builder().add_service(CoreServer::new(CoreService::new(
config,
sk.clone(),
attestor.clone(),
)))
}
}
- The macro does reduce the amount of code to be written but users might want the flexibility to specify their own config e.g. light client opts, etc.
- Users may also want to add more services.
- The macro is restricting users ability to use their libs of choice e.g. for CLI, tokio, etc. (I don't think we should make such decisions on their behalf)
Superseded by #123 (which has been merged already). |
Closes #81
Moved
quartz-cw
,quartz-enclave
, andquartz-proto
into:Also created
quartz_server!
macro to cut downenclave/main.rs
boilerplate down to 1 line:cycles-quartz/apps/mtcs/enclave/src/main.rs
Lines 26 to 30 in c3cbd80
Acceptance Criteria
This PR is ready when:
TODO
We don't need quartz relayer here right?