Improve error handling for legacy REST API endpoints when querying for non-amino signed Tx's

[clevinson]

As is currently implemented, all transactions signed with SIGN_MODE_DIRECT (which includes all tx's signed & broadcast via the CLI) are unqueryable via the legacy REST API endpoints (namely, /txs and /txs/decode endpoints).

The reason for this is because legacy REST endpoints all return amino JSON, and unfortunately a protobuf signed tx can't be represented in amino JSON. SIGN_MODE_LEGACY_AMINO_JSON is a mode which can be converted between the two transaction formats. But if you signed with SIGN_MODE_DIRECT which is protobuf native we can't convert the tx back to an amino tx.

At the moment, the error shown back to users when querying a proto tx via the GET /txs/$TX_HASH endpoint is:

{"error":"wrong SignMode. Expected SIGN_MODE_LEGACY_AMINO_JSON, got SIGN_MODE_DIRECT"}

In addition to #7636 (adding Deprecation headers to all legacy REST endpoints), we should probably explore a better way of handling this situation described above.

Here are a few options explored on discord already:

Option 1: Imrpoved error messaging

First option is to simply display more clear error handling here, and report back to the user something like

This transaction was created with thew new SIGN_MODE_DIRECT signing method,
and therefore cannot be displayed via legacy REST handlers, please use CLI or directly
query the Tendermint RPC endpoint to query this transaction.

This should be done for /txs, /txs/decode endpoints, and possibly other legacy end points that are expected to return amino JSON transactions.

Option 2: Decode whichever fields are possible

An alternative solution (as proposed by @ethanfrey on discord) involves converting the signed proto transaction to an unsigned amino Tx. This may be better UX behavior, as clients query the /txs endpoint only for the unsigned parts of that data.

The downside of this alternative, is that there are many new message types which do not support amino at all (like IBC). So this option would be inconsistent in which kinds of messages / transactions it could successfully decode & represent as amino.

Thx to @fadeev and @vince19972 for reporting this issue

[aaronc]

I'm not sure it would be worth the effort to take approach 2. Just delaying necessary changes

[ethanfrey]

We must guarantee perfect legacy support for any functionality that was available for cosmoshub-3

The entire work on legacy amino support is to delay eventual necessary changes. This is so 3rd parties - not just Lunie and BigDipper - but exchanges, validators, and the ledger app, have plenty of time to update. That is they will be able to update to a 0.40-based cosmos-hub without breaking any existing tooling. And then have the ability to slowly upgrade one by one while the cosmos hub supports both formats. Without this requirement, we would have not had any legacy amino support at all.

This tx decode error is a critical client side break

The broken functionality would cause a major error for exchanges using legacy software. If I send some ATOM to an exchange address using a cosmjs or Keplr based app (sign mode direct), and the exchange is unable to parse the transaction, what happens? My money just gets lost and not credited to my account? This would be a critical regression and exactly an example of what @zmanian spent the last few months trying to avoid. The two approaches need to live side-by-side for at least the entire 0.40.x series and cosmoshub-4.

SDK team should currently be 100% focused on ensuring 0.40 supports a successful cosmoshub-4

The fact that all work is on master and 0.40 seems like an after thought, even before a stable testnet is out using it, says something about the focus. I understand there are cool new features for 0.41, but before anything can move forward, we need to make sure there is a clean, tested and secure upgrade path for the Cosmos Hub and the entire ecosystem around it. Many months of work have gone into that, and without that, we will never have IBC on the Cosmos Hub.

[amaury1093]

We could also go with option 1, AND show the protoJSON of the decoded tx?

{
  "warning": "This transaction was created with thew new SIGN_MODE_DIRECT signing method...",
  "tx": {/* proto JSON*/}
}

[robert-zaremba]

If possible I would love to not extend the API and adding more amino support. It will only increase the maintenance scope and possible increase the burden with codecs we already have.

Kind of related to this discussion: https://discord.com/channels/669268347736686612/680435043570941973/768454232323260426

[aaronc]

We can return an unsigned transaction when it is just a sign mode problem and when amino is unsupported (i.e. ibc) return a better error message. Does that work @ethanfrey ?

[alessio]

I'd try to strike an agreement on the very principle of guaranteeing 3rd parties support, i.e.

3rd parties - not just Lunie and BigDipper - but exchanges, validators, and the ledger app, have plenty of time to update. That is they will be able to update to a 0.40-based cosmos-hub without breaking any existing tooling.

@aaronc @clevinson are we in agreement on that? Does anyone have particular concerns?
@zmanian any thoughts?

[aaronc]

I'd try to strike an agreement on the very principle of guaranteeing 3rd parties support, i.e.

3rd parties - not just Lunie and BigDipper - but exchanges, validators, and the ledger app, have plenty of time to update. That is they will be able to update to a 0.40-based cosmos-hub without breaking any existing tooling.

@aaronc @clevinson are we in agreement on that? Does anyone have particular concerns?

@zmanian any thoughts?

I feel like we are stretching the requirements here after the fact. The requirement was not "without breaking any existing tooling". It was not breaking hardware wallets

[aaronc]

We made efforts to support legacy endpoints as much as possible which I believe is the case here. But I was never aware of a 100% compatibility guarantee for all existing software. Hardware wallets was the requirement.

[alexanderbez]

Im in favor of simply returning a meaningful error. We cannot and did not make any guarantees around legacy API behavior when it comes to working with data generated by new functionality.

[clevinson]

This issue was discussed on the Architecture Review call (notes here), and the decision was to work on the following solution for v0.40.0:

• Proceed with a limited version of Option 2 as described in this issue:
  • implement a workaround so legacy REST endpoints decode proto Tx's when the only failing component is the proto signature (e.g. no workaround for ibc transactions) Return an unsigned tx in legacy GET /tx endpoint when signature conversion fails #7649
  • Add more clear error handling that points to specific documentation for migrating to the new APIs
• adding grpc & grpc-gateway endpoints for TxByHash Add gRPC endpoints for querying txs, TxsByEvents, tx/broadcast #7355
• add deprecation headers for all legacy amino REST endpoints Add HTTP Deprecation header to legacy REST endpoints #7636

[alexanderbez]

I just want to say I'm opposed to Proceed with a limited version of Option 2 as described in this issue: but proceed anyway 👍

[clevinson]

Moving this back to the backlog and assigning to @anilcse for the remaining tasks here.

We still need to create a more human readable error whenever legacy REST queries fail due to inability to convert from proto tx to legacy amino tx.

This will mainly occur during queries to legacy REST tx endpionts where the transactions being queried or decoded have messages that are not amino compatible (eg. ibc).

The error message should communicate clearly that proto messages cannot be read by legacy endpoints, and point to relevant grpc / grpc gateway endpoints if possible.