0xPolygonMiden / miden-node

Reference implementation of the node for the Polygon Miden rollup
MIT License
53 stars 37 forks source link

Remove block producer's Store trait #196

Open hackaugusto opened 9 months ago

hackaugusto commented 9 months ago

The Store trait abstracts over the gRPC server, the main utility of this trait is to implement tests. However, this is not necessary, since tonic already has a strategy to mock the servers for testing, here is an example:

https://github.com/hyperium/tonic/blob/master/examples/src/mock/mock.rs

bobbinth commented 9 months ago

Would we basically just replace Store trait with the current DefaultStore struct? (i.e., Store trait goes away and DefaultStore struct becomes Store struct).

If we can do this while still mocking store for test purposes, I'm in favor or that (upon a quick look at the example, I didn't quite understand how it would work, but I also didn't look too long).

hackaugusto commented 9 months ago

Implementation wise it would something like this:

bobbinth commented 9 months ago

Makes sense. Yes, I think this works. @plafer what do you think?

plafer commented 9 months ago

The one downside is that it makes it harder to move away from gRPC in the future. But if that's not much of a concern, then yes this approach is simpler and thus better.

plafer commented 9 months ago

I'm just remembering previous conversations we had about how when all components are running in the same process (including the database and rpc), we shouldn't need to use gRPC. This change would force us to keep using gRPC in that situation.

hackaugusto commented 9 months ago

The one downside is that it makes it harder to move away from gRPC in the future. But if that's not much of a concern, then yes this approach is simpler and thus better.

This is a fair point. But I think changes like this happen once or twice in a project's lifetime. So personally I would optimize for easy of readability here.

I'm just remembering previous conversations we had about how when all components are running in the same process (including the database and rpc), we shouldn't need to use gRPC. This change would force us to keep using gRPC in that situation.

This is true, we probably don't need the socket api, but we would need to do something similar to what is done in the tests, and communicate over a channel.