reorganize basecli appTx commands

[rigelrozanski]

I wanted to make the process flow a bit more clear when generating a new AppTx transaction. Rather than passing around an AppTx struct to be have flags directly inserted into it, flags are parsed and fed to back up to the custom plugin app-tx command and then added to an AppTx struct as it's created.

Additionally a few more commands that an plugin will typically not need to consider when developing the appTx have been clumped together and exported to the bcmd command (BroadcastTx)

Lastly, one nuance implication of these changes is they are more conducive to more complex multi-transaction commands which a developer may wish to implement. The common flags can be pulled as variables and applied to multiple transactions this way instead of needing to cross reference the previous transaction which appTx flags were pulled into.

[ethanfrey]

Nice cleanup.

Can you also check other repos (tmlibs/cli, tendermint, light-client, basecoin-example...) to see if we use this naming format FlagXXX and raise issues if not, so we can standardize (or maybe it is just my code that is different).

Also, please let me know what you think of the PrepareAppTx idea. If you like it, let's just make that small enhancement as another pr (or right on the branch).

[ethanfrey]

Not needed, as if found == false, LoadJSON will have done no work and err is guaranteed to be nil. So I optimized to use only one err checking block.

But this is more clear, and less likely to have subtle errors later introduced.

[rigelrozanski]

Okay, for more clarity use a nil assign found, _ :=

[ethanfrey]

no, if found is true, then err is what we want

it made sense to me as i know the semantics of the function (and wrote all three cases in the header), but you are right, it is confusing. leaving it the way you put it.
one if statement costs the computer <1ns, but the lack of it costs a developer several minutes.

[ethanfrey]

Sure, I guess this is the more standard naming?

We should make sure we put Flag first in all repos then.

[rigelrozanski]

Yes, this naming convention makes more sense, I will explore this issue of standardization across our repos, will also add to the coding standards documentation

[ethanfrey]

Yes, good helper. Creating the tx from input is app-dependent, as is the output logic, but this should be the same for almost every app.

[ethanfrey]

Okay, seems a bit more verbose to me, but I see how you use it in the counter app.

If this usage seems more clear to you (no hidden modifications of the AppTx), then I am cool with it as well.

[rigelrozanski]

Yes, this is definitely my preference - another advantage I've been discovering while programming tracko is that the senders address is needed in the generation of the app.Data - it's nice to not need to reach into an incomplete transaction to pull this out and just use txInput.Address much more clear

[ethanfrey]

great argument. glad you are putting this to the test to make a better api. more useful and less magic

[ethanfrey]

@rigelrozanski : possible improvement...

If we want to get crazy with helper functions. It seems that everything from here until BroadcastAppTx is the same on every app.

So in addition to BroadcastAppTx, we could have PrepareAppTx(name, data) (*types.AppTx, error) that creates the apptx. Maybe even combine them into PrepareAndBroadcastAppTx(...).... but then again, maybe that is going overboard and would obscure the funcionality

[rigelrozanski]

I had actually originally set this up to go Cr4zY with the helper functions, but I decided that it makes sense to have some of the process still encapsulated in the application command - This is specifically in reference to the many variations of process flow I envisioned. There are really 4 elements here:

• Get standard flags
• Get plugin-specific flags
• Create/Broadcast AppTx
• Output
  Already while programming trackotron I've noticed how the generating the plugin-specific flags can require niche information from processed standard flags, I supposed we can pass in all the information in the form of a semi-complete AppTx - but this seems messy and unnecessary. In more complex (maybe multi-transaction) commands, some of these variables may be reused or reassigned before added to an AppTx and broadcast, and lastly a command may want to use different output.
  This tiny bit more verboseness ultimately makes the code much more flexible/clear

[ethanfrey]

👍

[ethanfrey]

Thank you for the sanity check ;)