Add proof state subcommand

[rigelrozanski]

This is equivalent to RegisterQuerySubcommand within the heavy client, allows a space to have custom plugin proofs for custom flags and more complex responses.

Closes #13

[ethanfrey]

Let's chat Monday so I can better understand the reasoning behind it, so I can review properly.

I agree we need to make some changes here, just want to make them clean.

[rigelrozanski]

@ethanfrey checkout its usage here: https://github.com/tendermint/trackomatron/blob/master/cmd/trackocli/proof.go
We're piggybacking on the state get function to build complex query operabilities

[ethanfrey]

Notes from the chat...

Should invert the flow a bit more to allow registering custom flags for queries...

One point is commands/common.go: GetCertifier() as a way to get common objects from command flags.

Get by primary key is very different than lists by provability.

Let's add a new subcommand to trackocli to test this stuff...

trackocli list [arg...]

And experiment there on how to prove it.

[ethanfrey]

How to prove a list?

• List of invoice keys is stored under one key

• I can prove this list is valid

• But I have no info on how to filter

• I can load all invoices in system, check each against the filter, and return matches

• This can be done on server fast, or clients very slow

• If we have eg. 1 million invoices in the system, even server side is slow.

• To prove, add secondary indexes as embedded merkle trees.

[ethanfrey]

Address individual get queries (not lists):

They sometimes have attributes, like --save-dir to where to save it

Plugin needs to have full access to register it's own command.

[rigelrozanski]

Checkout some code! I've added proof state list to this PR as well as implemented relevant changes in trackomatron. Additionally, I really cleaned up the way that the custom lightclient plugin query functionality within trackomatron interacts with the functionality of light-client, it mostly is just using a revised version of the GetProof function located in light-client/commands/get.go

[ethanfrey]

Okay, I think this needs a bash test. But from the review, I am confused by the flow. This is mainly due to my convoluted scheme to start with that you extended....

in trackocli/main.go:

	proofs.StateGetPresenters.Register(trcmd.AppAdapterProfile, adapters.ProfilePresenter{})
...

in trackocli/proof.go:

https://github.com/tendermint/trackomatron/blob/master/cmd/trackocli/proof.go#L14
https://github.com/tendermint/trackomatron/blob/master/cmd/trackocli/proof.go#L37-L42
https://github.com/tendermint/trackomatron/blob/master/cmd/trackocli/proof.go#L70-L72
https://github.com/tendermint/trackomatron/blob/master/cmd/trackocli/proof.go#L88-L98

In commands/query.go:

https://github.com/tendermint/trackomatron/blob/master/commands/query.go#L339-L359

This works, but seems way too complex. And basically cuz you didn't want to rip apart my convoluted system.

[ethanfrey]

I want to make a proposal... let's see if I have time.

[ethanfrey]

Okay, I made a new branch proofsubcmd-ethan, please review it and if you like it, merge into this branch. Make any more changes you want (after trying out on trackocli), and then I will review again.

Please just register the list command top-level in trackocli and keep it out of light-client until we figure out a proper way for handling proofs. This "advanced functionality" will not ship as part of light-client until it is vetted. Just commands we are confident in.

You can use this as a basis for all your query subcommands:
https://github.com/tendermint/light-client/blob/proofsubcmd-ethan/commands/proofs/state.go

import (
    "github.com/tendermint/light-client/commands"
   proofcmd  "github.com/tendermint/light-client/commands/proofs"
    "github.com/tendermint/light-client/proofs"
)


var profileCmd = &cobra.Command{
	Use:   "profile [name]",
	Short: "Get the profile by name",
	RunE: doProfileQuery,
}

func init() {
  proofcmd.RootCmd.AddCommand(profileCmd)
}

func doProfileQuery(cmd *cobra.Command, args []string) error {
	// parse cli
	height := proofcmd.GetHeight()
        if len(args) != 1 || args[0] == "" {
           return errors.New("invalid profile name")
        }
       key := invoicer.ProfileKey(args[0])

	// get the proof -> this will be used by all prover commands
	node := commands.GetNode()
	prover := proofs.NewAppProver(node)
	proof, err := proofcmd.GetProof(node, prover, bkey, height)
	if err != nil {
		return err
	}

        profile, err := invoicer.GetProfileFromWire(proof.Data())
	if err != nil {
		return err
	}

	// we can reuse this output for other commands for text/json
	// unless they do something special like store a file to disk
	return proofcmd.OutputProof(profile, proof.BlockHeight())
}

[ethanfrey]

I think that is less boiler-plate and less confusing than the current approach. And you have full control of the command parsing.

I register cmd query tx [txhash] by default, which auto-determines the data from all registered tx types. I also register cmd query key [hexkey] which does raw hex-binary lookup in the app.

All other query subcommand names belong to your app.

[ethanfrey]

The only flag that cmd query takes by default is --height, which you can read with proofcmd.GetHeight(). My examples take one arg in hex, you can do any arg and flag parsing you wish. And if you want to save to disk, just don't call proofcmd.OutputProof (default print json), but your own output handling.

[rigelrozanski]

With regards to your first comment that references code from trackomatron, be aware that the reason DoQuery... functions are referenced is not to fit into your system here but to allow for the same code to be called from the heavy client and the light client. -> it really needs to be this way, The custom functions which query the ABCI app are fundamentally different between the light and heavy client, however all the other code is the same, therefor it really makes sense to just pass in the function that must be called for query actions and let everything else remain.

The code that you've described in your second comment will generate unnecessary code duplication. I've reduced the abstraction as much as possible within the latest commit - When I reflect it's really only one layer of abstraction more complex than the code you presented... pretty straight forward/clean, but yeah slightly more confusing than your example but eliminates unnecessary code dup.

Lastly, your code updates all look good, I fully support removing the Presenters, doesn't make sense when you can have the app deal with it!

[rigelrozanski]

Alright revised trackotron plan based on our conversion: Consolidate query and transactions into the light client, add the whole light client command as a subcommand of the full server node - This should make the codebase more digestible - this code dup issue becomes non-existent.

[ethanfrey]

Looks good. I am happy with how it turned out.

Very productive conversations on design. Looking to get this cli in tip-top state :)