Work around removal of broadcast-mode "block" in cosmos-sdk 0.47

JimLarson commented 8 months ago

What is the Problem Being Solved?

The agd CLI argument --broadcast-mode=block will no longer be supported when we upgrade to cosmos-sdk v0.47 (planned for upgrade-16). This is extensively used in scripts, comments, and documentation.

Description of the Design

The quickest and most straightforward solution is to just restore the feature, restoring the short plumbing between the site that dispatches on the broadcast-mode flag value to RPC call. That way we don't have to try to track down all callers or expand to the larger sequence of replacement actions - which aren't available until 0.47.

Security Considerations

N/A

Scaling Considerations

N/A

Test Plan

We can restore a deleted unit test to ensure compatibility. We have sufficient CI coverage in pretty much all tests that submit transactions to cover end-to-end.

Upgrade Considerations

N/A since we're providing compatibility.

mhofman commented 7 months ago

We had discussed that for swingset tx, we could also provide an option to "block" until the transaction is executed by swingset, which assumes unified reporting of tx results to vstorage (#7148). This is clearly out of cope here and a potential follow up only

JimLarson commented 7 months ago

After discussion, the immediate plan is to restore the "block" option by modifing cosmos-sdk. Since the call sites are hard to identify, and the most convenient stock cosmos-sdk command to use as a workaround wasn't introduced until 0.47, it makes sense to take on the fork maintenance rather than trying to change all usages atomically with the release. We can maintain compatibility either by issuing the same cometbft-level RPC request (must ensure it's supported with the same semantics in future releases) or by issuing the websocket RPC to subscribe to the event stream for the execution (see event-query-tx-for implementation). Either way, compatible output should be generated by the agd tx command.

JimLarson commented 7 months ago

After 0.47 is deployed, call sites have the option of switching to broadcast-mode sync if they don't actually process the results, or expanding the interaction to use the standard cosmos-sdk API, where agd tx ...args... --broadcast-mode="block" becomes:

agd tx ...args-except-signing-and-broadcast-args... --generate-only >unsigned.json
agd tx sign ...sign-args... <usnigned.json >signed.json
agd query event-query-tx-for <hash> &
agd tx broadcast signed.json
wait
(remove files, handle errors, possibly massage output)

However, the effort of doing this elaborate expansion at each call site is probably much more than the maintenance effort of keeping compatibility in the cosmos-sdk fork.

JimLarson commented 7 months ago

Various options were considered to provide compatibility, or near-compatibility, through a wrapper layer outside cosmos-sdk, such as:

A new top-level subcommand of agd tx-plus which detects the use of block mode and implements the above expansion.
A wrapper script agdtx which invokes agd in the above expansion.
An initial step in agd itself to parse its command line, recognize the deprecated broadcast-mode flag (and maybe other compatibility fixups, e.g. submit-proposal), or fall through to normal command line processing.

These would allow minimal change, or no change, at each call site, rather than the more difficult expansion above.

But all of these have some burden in correct command-line parsing. If you have a command line agd -a b -c d -e f, you need to know whether the flags take arguments or not to know whether it's subcommand b d f or b f or d, etc. Even worse, the allowed flags and their properties depend on the subcommand parsed so far. One could hope to have the cobra command library do this parsing independent of execution, or give enough introspection into its data structures to allow an analysis, but it doesn't seem to provide that. Replicating the argument interpretation outside cosmos-sdk introduces a maintenance burden, even though the cosmos-sdk code itself isn't modified. Again, it seems simpler to intercept at the broadcast-mode dispatch site.

JimLarson commented 7 months ago

Richard points me to an illustration that the cobra library does contain sufficient introspection to be able to handle the argument parsing with a fairly low maintenance overhead. See Go Playground. There's still the issue of segregating the flags and effecting the desired sub-functionality from the outside. Overall, the quick fix to just restore the "block" option at the broadcast mode dispatch site seems simpler, even with the maintenance of the fork, at least in the short run. But if the underlying CometBFT RPC method gets deprecated, or if we tackle #7148, this cobra introspection seems like a leading solution.

Agoric / agoric-sdk