Can't parse Cosmos transactions with empty Denom anymore

[penso]

This PR #470 breaks parsing successful Stargaze transactions (and probably other chains) like this one (mintscan) with:

cosmrs::Tx::from_bytes error: invalid denomination: ""

Anything earlier than this commit works. This code sample will show you how to reproduce the issue:

#[test]
pub fn denom_error() {
    unsafe {
        std::env::set_var("RUST_BACKTRACE", "1");
    }

    let block_url = "https://rpc.full-nodes.stargaze-1.stargaze.build/block?height=1931505";

    let response = reqwest::blocking::get(block_url).expect("Can't fetch block");

    let data: serde_json::Value = response.json().expect("Can't get block JSON");

    let Some(txs) = data
        .get("result")
        .and_then(|c| c.get("block"))
        .and_then(|c| c.get("data"))
        .and_then(|c| c.get("txs"))
        .and_then(|c| c.as_array())
    else {
        return;
    };

    for (idx, tx) in txs.into_iter().enumerate() {
        let tx = tx.as_str().expect("Can't get tx str");

        #[allow(deprecated)]
        let bytes = base64::decode(tx).expect("Can't decode");

        let res = cosmrs::Tx::from_bytes(&bytes);
        let hash = sha256::digest(bytes.as_slice()).to_uppercase();

        match res {
            Ok(_) => {
                println!("[{}] [{hash}] ok", idx);
            }
            Err(e) => {
                println!("[{}] [{hash}] error: {:?}", idx, e);
                panic!("Transaction can't be parsed");
            }
        }
    }
}

Error:

   13 ---- denom_error stdout ----
   14 [0] [D15627C06F05575A6840B06631260AD563A6BD503CDA58F38AB35F76A10D9550] ok
   15 [1] [C810347BC8C97F5883C8213289DDBF3E526A8F392605EE5EB84D9AAC6ED06F8B] ok
   16 [2] [D3EFB0314EACB63C3689749D943A5F08D6FE804B83574800CA033BE932A88666] error: invalid denomination: ""
   17
   18 Location:
   19     /home/penso/.cargo/git/checkouts/cosmos-rust-5eb3768b2a88fb5e/ced74a7/cosmrs/src/base/denom.rs:36:60

[penso]

BTW I believe we should have a test framework available to add tests for such issues and prevent regressions. Something to easily add a block height, or a transactions, and the test framework to fetch that data from nodes, cache it locally to prevent depending on outside nodes when running tests, and then do whatever you want to test.

I added something like this in Constellations and would gladly add a similar feature on cosmos-rust. Is anyone "sponsoring" such work?

[tony-iqlusion]

We already have basic integration tests here: https://github.com/cosmos/cosmos-rust/blob/main/cosmrs/tests/integration.rs

Those are in addition to the unit tests for Denom.

If you would like to improve either of those somehow, that would be great.

However, it's somewhat surprising to me that the empty string is allowed as a Denom. Notably the Cosmos SDK logic seems to disallow denoms less than 3 characters long:

https://github.com/cosmos/cosmos-sdk/blob/6a07568/types/coin.go#L890

Do you know why that logic isn't being triggered in e.g. Stargaze?

(And note, generally, making any validation logic stricter runs the risk of breaking things that were working, and there's no way across the plethora of Cosmos chains out there that we can account for everything every chain is doing)

[penso]

I don't think Stargaze is doing anything specific on that level, it just uses the cosmos-sdk as it is. Either the code path didn't trigger that validation, or that validation was added at a later time than this block. I wouldn't be surprised to find this same bug on other chains.

What I've learned from indexing multiple cosmos chains is you can't expect anything, see all my previous filled up bugs. I keep finding issues related to data correctness.

[tony-iqlusion]

If you think there’s no value to cosmrs maintaining invariants about anything at all whatsoever, even the same ones found in the Go implementation of the Cosmos SDK, we should just give up, completely abandon cosmrs and have people use cosmos-sdk-proto directly so we never ever have to deal with issues like this again.

But I would rather get to the bottom of cases where Cosmos SDK maintains invariants that chains are ostensibly violating.

There's a long history in protocol design of trying to tolerate everyone doing what they want and it lead to an interoperability nightmare.

[penso]

Sorry if I sounded negative, wasn't my intention and I very much value your work and the work done within cosmrs. I was just mentioning than based on me trying to index multiple chains since genesis, I had issues as you're aware like those signature header tags, or those 00.. for validator keys. And I don't see how to prevent that other than being more "loose".

I'm going to parse all chains I have blocks for to see if that's a Stargaze specific issue or if that happens on other chains as well.

[penso]

Issue is when you have a 0 gas fee, on-chain has an empty string for denom:

{
  "signer_infos": [
    ...
  ],
  "fee": {
    "amount": [
      {
        "denom": "",
        "amount": "0"
      }
    ],
   ...
  }
}

My first guess is Go JSON parse that as default values and doesn't go through the validation. So the Rust validation should probably not run if you have a "default" Coin (0 amount, empty string).

[tony-iqlusion]

I've tried to add support for this in #479

[penso]

I've tried to add support for this in #479

I confirm this fixes the issue, thanks!