mobilecoinfoundation / mobilecoin

Private payments for mobile devices.
Other
1.16k stars 151 forks source link

Refactor `mc-connection` and `fog-enclave-connection` to unify design #1378

Open jcape opened 2 years ago

jcape commented 2 years ago

Right now there is the FooConnection API set, which lives in the mc-connection crate and provides the client APIs used by, e.g. mobilecoind and full-service. Separately, there is a stack of fog connection APIs which was written independently and follow a completely different style.

We should unify these two APIs into one style of API, so we don't have to port fixes between them. It may be useful to make this "new" API async-friendly.

cbeck88 commented 2 years ago

The reason all the fog connections were done differently is that they have a different requirement -- we need to share code across different enclave connection types, which all use the same "auth / encrypted request" communication pattern for attestation. All the mobile clients and SDKs have some form of generic programming used for this because otherwise you end up copying 4 times all the attestation logic. Additionally, fog lived in its own repo for a long time.

The solution in rust code is trait EnclaveGrpcChannel:

https://github.com/mobilecoinfoundation/mobilecoin/blob/a106238a4cb56378e9fb9a859ea33873223991d7/fog/enclave_connection/src/lib.rs#L39

and an implementation of this can be used in EnclaveConnection:

https://github.com/mobilecoinfoundation/mobilecoin/blob/a106238a4cb56378e9fb9a859ea33873223991d7/fog/enclave_connection/src/lib.rs#L53

which generalizes the ThickClient object in mc-connection: https://github.com/mobilecoinfoundation/mobilecoin/blob/a106238a4cb56378e9fb9a859ea33873223991d7/connection/src/thick.rs#L117

I think the right approach here is:

cbeck88 commented 2 years ago

This doesn't address the issue of creating async APIs -- it may be logical to address this work in multiple discrete steps.

wjuan-mob commented 2 years ago

There is also a set of state transition logic similar to both of these in the android_bindings crate. E.g. see: Java_com_mobilecoin_lib_AttestedClient_attest_1start

The way the java stuff works is that we have an attestedClientState that is passed in from a java object. There is a pointer to the object that is stored in a long. See take_rust_field

Android cannot do self.authenticated call from the fn attest(&mut self) -> StdResult<VerificationReport, Self::Error> function, it needs to take the result of that call from the android side and pass it in. Therefore the logic needs to be split to be used by android.

Ideally top level enclaveconnectionstate that handles ready etc transitions. It takes some common set of traits for various connection apis. Minimize the amount of duplication between these 3 locations.