Reimplement merkleeyes #211
Conversation
ethanfrey
force-pushed
the
feature/reimplement-merkleeyes
branch
from
0f7f495 to
09b1bb3
Compare
| return stack.New( | ||
| base.Logger{}, | ||
| stack.Recovery{}, | ||
| ). |
There was a problem hiding this comment.
Just to clear my understanding - eyes will run concurrently to a basecoin instance and therefor needs it's own stack and logger
There was a problem hiding this comment.
Is this set of commands for if you wanted to run an eyes client out of process from basecoin? Can we run it in process? if yes, when do we even want out of process functionality?
There was a problem hiding this comment.
Yes, to run it as a separate process. Mainly for testing.
Currently merkleeyes is used for two things. A storage layer for base coin (no more in unstable) and a backend abci app for testing tendermint.
This would replace the second usage and consolidate all abci apps in the new framework.
cmd/eyes/main.go
Outdated
| // out own init command to not require argument | ||
| InitCmd, | ||
| commands.StartCmd, | ||
| //commands.RelayCmd, |
There was a problem hiding this comment.
What's happening with Relay? Did you mention that we don't need any more? Should delete all instances of this line if so
There was a problem hiding this comment.
Totally right, not needed here
In the main app, still need to figure out how
cmd/eyescli/main.go
Outdated
| // EyesCli - main basecoin client command | ||
| var EyesCli = &cobra.Command{ | ||
| Use: "eyescli", | ||
| Short: "Light client for tendermint", |
There was a problem hiding this comment.
Capitalize Tendermint (as per coding standards) - should only be lowercase ever when it's a part of the cli-command name (Use) or file/folder name
modules/etc/commands/query.go
Outdated
| Use: "etc [key]", | ||
| Short: "Get data stored under key in etc", | ||
| RunE: commands.RequireInit(etcQueryCmd), | ||
| } |
There was a problem hiding this comment.
what's etc again? Maybe we should add more documentation - etc not very descriptive. If this command is supposed to just be querying raw data from the eyes (in general, as this commands code-comment suggests) then etc is maybe a bad descriptor of the functionality - however Short: desc seems to suggest otherwise
There was a problem hiding this comment.
I need naming help.
It is like etcd that bucky refers to a lot.
It is also like the merkleeyes app, thus eyes.
Maybe a different name?
I need help naming for sure
|
Overall looks good - main concern is just understanding |
modules/etc/tx.go
Outdated
| RegisterImplementation(SetTx{}, TypeSet, ByteSet) | ||
| } | ||
|
|
||
| // SetTx sets a key-value pair |
There was a problem hiding this comment.
Let's get in the habit of ending our godoc sentences with a period.
modules/etc/handler.go
Outdated
| return | ||
| } | ||
|
|
||
| // doSetTx write to the store, overwriting any previous value |
modules/etc/handler.go
Outdated
| store.Set(tx.Key, wire.BinaryBytes(data)) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Empty result is okay with no extra info. It is standard in abci but should be commented
modules/etc/commands/tx.go
Outdated
| RunE: commands.RequireInit(removeTxCmd), | ||
| } | ||
|
|
||
| //nolint |
There was a problem hiding this comment.
What's the nolint for here? just curious.
There was a problem hiding this comment.
cuz i was too lazy to comment both exported const.
otherwise the automatic linter complains :(
tests/cli/eyes.sh
Outdated
| export EYE_HOME=${SERVE_DIR} | ||
| ${SERVER_EXE} init --chain-id=$CHAIN_ID >>$SERVER_LOG | ||
| startServer $SERVE_DIR $SERVER_LOG | ||
| if [ $? != 0 ]; then return 1; fi |
There was a problem hiding this comment.
These can be written [ $? = 0 ] || return 1 -- but this way is ok too.
cmd/eyescli/main.go
Outdated
| // These are default parsers, but optional in your app (you can remove key) | ||
| query.TxQueryCmd, | ||
| query.KeyQueryCmd, | ||
| // this is out custom parser |
modules/etc/store.go
Outdated
| // Data is the struct we use to store in the merkle tree | ||
| type Data struct { | ||
| // SetAt is the block height this was set at | ||
| SetAt uint64 `json:"created_at"` |
There was a problem hiding this comment.
Should we make the go and json keys match?
Notice that everything has json tags and that we check price in CheckTx, while we run real code in DeliverTx. Return values are meant for the client.
Note how the all framework commands can be reused with a bit of configurations. And one can add the custom query and tx commands.
ethanfrey
force-pushed
the
feature/reimplement-merkleeyes
branch
from
36a2ba7 to
ade9d45
Compare
To provide a semi-useful demo app for tutorials,
to document the process to do so with the git comments and commits,
and to allow us to deprecate merkleeyes app in the repo....
I implemented a simple k-v store interface as a basecoin/basecli app.
Please take a look.