Add NodeInterface #166
Add NodeInterface #166
Conversation
Because Node is used directly (in lazyledger-core and cosmos-sdk), there is no easy way to replace it. Introduced interface opens a way to replacing Tendermint node in cosmos-sdk easily.
| // don't run in parallel, or try to simulate an entire network in | ||
| // one process... | ||
| func New(node *nm.Node) *Local { | ||
| func New(node nm.NodeInterface) *Local { |
There was a problem hiding this comment.
Is this cause you would like to reuse the RPC in optimint?
There was a problem hiding this comment.
I think that by reusing RPC have following pros:
- easy integration with cosmos-sdk (see Use Optimint node instead of Tendermint Node cosmos-sdk#20)
- don't need to reinvent the wheel
Do you see any cons?
There was a problem hiding this comment.
I agree with you, dont see any cons outside the known issues within the RPC layer will be known issues in optimint. They mainly stem from event streaming and mutex contention (may not happen in optimiment)
|
Is there a reason why optimint can't simply use the Node struct, instead of defining a new NodeInterface interface? That way you can just have a function called NewOptiNode or similar that creates an Optimint Node, and also use that in cosmos-sdk. https://github.com/lazyledger/lazyledger-core/blob/8a8b8b5483ba4fc5fcc6c25452135417df539f37/node/node.go#L184 Edit: I assume most of the fields in the Node struct are only used by Tendermint. |
|
Using a tendermint Node would have been the right approach if we decided to go with using tendermint and ripping out / stripping down the consensus reactor. The reactors are fields in Node struct and can be swapped out. Using an interface gives us absolute freedom on the optimint side though. That said, It would be possible to use the Node struct but it would be much more difficult to get right and the interface approach seems cleaner. As a general remark: is this choice documented somewhere? I guess it's OK, to not use ADRs but we should certainly weigh pros and cons for choices like this. A summary in an issue, or an ADR, or a design document. Also, there might be other ways to achieve the same. Their pros/cons should also briefly be considered. |
I agree and I would suggest to ping the SDK team on discord about this, or tag @robert-zaremba here and in the SDK pr. |
|
As agree it will be much cleaner to create own node type, instead of hacking with tendermint one. This was the reason behind my decision. I changed target of this branch to |
| ConfigureRPC() error | ||
| GetLogger() log.Logger | ||
| EventBus() *types.EventBus | ||
| } |
There was a problem hiding this comment.
If feasible somehow it would be nice, if we could move this interface to where it is consumed and needed. Namely into the SDK. I have not thought this through and might not be feasible. If so, we can simply merge this here now and iterate from there.
Either way, it seems unlikely, that this gets upstreamed any time soon.
There was a problem hiding this comment.
I'm closing the PR, and working on updates to other two.
4 participants
Because Node is used directly (in tendermint/lazyledger-core and cosmos-sdk), there is no easy way to replace it.
Introduced interface opens a way to replacing Tendermint node in cosmos-sdk easily.
It would be great, if we can push similar change upstream.