Uh oh!
There was an error while loading. Please reload this page.
Drop-in replacement of Tendermint Node - #13
Conversation
Please remove or revert this commit later
tzdybal
commented
Feb 21, 2021
| "github.com/lazyledger/lazyledger-core/types" | ||
| ) | ||
| var _ tmnode.NodeInterface = &Node{} |
There was a problem hiding this comment.
This line might need to change, if we address celestiaorg/celestia-core#166 (comment) as the interface would live in the SDK. Hmm, actually: this might not be the smartest decisions either. Because then optimint would import the SDK and the SDK would import optimint.
Can't these assertions live in the SDK either?
There was a problem hiding this comment.
Either way, let's not overthink this now. We can also merge all 3 PRs as is and figure out the proper abstractions as we iterate over the code. Also, adding in features might also change how we think about this.
There was a problem hiding this comment.
This assertion is only to ensure that we're not braking anything during development.
As we're moving to scaffolding based approach and try to avoid all changes in tendermint/lazyledger-core, we can get rid of this interface at all. or keep it here just to reflect the requirements from SDK.
Event bus subscription is missing. This is glue-code requred for RPC binding with cosmos-sdk (it's direct replacement of tendermint's rpc/client/local).
This implementation is in line with Tendermint implementation (only slightly refactored).
Uh oh!
There was an error while loading. Please reload this page.
liamsi
left a comment
There was a problem hiding this comment.
Left some preliminary comments.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| func (l Local) ABCIInfo(ctx context.Context) (*ctypes.ResultABCIInfo, error) { | ||
| return core.ABCIInfo(l.ctx) |
There was a problem hiding this comment.
I did't try this myself and maybe I'm missing some context but wouldn't this code always panic? As far as I understand most of the core functions rely on a global var called env, which is never initialized here (and hence it will be nil). See for example the implementation of ABCIInfo:
// ABCIInfo gets some info about the application.// More: https://docs.tendermint.com/master/rpc/#/ABCI/abci_infofuncABCIInfo(ctx*rpctypes.Context) (*ctypes.ResultABCIInfo, error) {
resInfo, err:=env.ProxyAppQuery.InfoSync(ctx.Context(), proxy.RequestInfo)
iferr!=nil {
returnnil, err
}
return&ctypes.ResultABCIInfo{Response: *resInfo}, nil
}The same holds for all the other methods too.
Oh, I see, it would panic anyway as the ConfigureRPC method does panic itself - and it would be the place where the env would be created. As you have the proxyApp at hand already would it make sense to set the fields that you can via n.proxyApp.Query() and n.proxyApp.Mempool()? At least they seem to be independent of the rest of the Environment.
There was a problem hiding this comment.
Thanks for pointing this out. Actually, I decided to stop using rpc/core entirely. I hope it will result in cleaner code.
For now (727619b):
- implemented methods that depends only on
proxyAppandEventBus - other methods throws panic
- tried to note what's necessary for each method to be implemented
After re-thinking, I decided not to use `rpc/core` package from lazyledger-core. * it will be easier/less hacky to provide own components (like mempool) * I'm not a big fan of global `env` in `rpc/core` * this way there is a clean boundary between lazyledger-core and optimint implementations * functions that can be implemented using ProxyApp and EventBus are already implemented * I made comments in each function, about what is necessary to implement it
3e0240a to
6c333f7Compare
While there are no tests yet, I added some dummy code to
OnStartto ensure that I optimint node can communicate with App via ABCI.This resolves#1.