search

tendermint / light-client

DEPRECATED: A light client for tendermint, supporting signatures, proofs, and validation (see github.com/tendermint/tendermint/lite)
Other
17 stars 9 forks source link

Make tx plugin registration more flexible #22

Closed rigelrozanski closed 7 years ago

rigelrozanski commented 7 years ago

It would be really nice to restructure the tx subcommands in the same way as introduced for query in https://github.com/tendermint/light-client/pull/21 which allows for complete autonomy from the plugin side - aka we pass around a plugin command which the plugin defines, this way all the flag management can be taken care of on the plugin side and not be registered like they are being currently (this would also allow for args in a plugin tx command which is currently not allowed by the rigid framework prescribed by light-client)... Not to mention we should strive for consistency for how plugin subcommands are registered. what do you think?? I could give it a stab If you like the idea @ethanfrey

ethanfrey commented 7 years ago

This is about creating new TX, right? Not the proof, query stuff.

After we merge your pr, let's talk this through. We can maybe use a similar approach.

I think I went overboard in abstracting out the commands, and if you like the query approach, we can discuss something similar here

rigelrozanski commented 7 years ago

Okay, I just got heavy into this code, realized that there are a few things I'm not totally comfortable modifying yet, BUT this is my idea... I think we can keep a similar process flow, but rather than passing in the app name and flags with txs.Register, we could pass in a whole command right there! The the RunE term of that command could be updated as it's already defined. But maybe we can even get simpler and follow the same process flow as you're suggesting. I'm fully in support of this too. Talk more after this merge

To put it lightly, I think the way that it's abstracted right now is a bit insane. hahahaha

ethanfrey commented 7 years ago

To put it lightly, I think the way that it's abstracted right now is a bit insane. hahahaha

Yup, this is why I need someone to use my code. Not so much to see if it works correctly, but if it works in a way that makes sense to anyone else...

ethanfrey commented 7 years ago

btw, i want to add all these changes to the basecoin pr... once we fix this up.

let's make all updates to new light-client changes in the one pr: https://github.com/tendermint/basecoin/pull/107

ethanfrey commented 7 years ago

Agreed... not sure what i was thinking when i wrote this... probably reading too much math at the time.

rigelrozanski commented 7 years ago

Okay agreed one PR

ethanfrey commented 7 years ago

Fixed with this merge and also implemented in basecoin in https://github.com/tendermint/basecoin/pull/107