manuelmauro / algonaut

A rusty sdk for Algorand.
MIT License
64 stars 38 forks source link

Indexer appears to return the AAAA... "zero" address instead of no address at all (e.g. `undefined`, `null`, or simply absent) for asset information. #142

Open AlterionX opened 2 years ago

AlterionX commented 2 years ago

Describe the bug The semantics of Option<Address> seems invalid for assets.

To Reproduce Attempting to get the json from https://algoindexer.testnet.algoexplorerapi.io/v2/assets/78377817 yields:

{
    "asset": {
        "created-at-round": 20385749,
        "deleted": false,
        "index": 78377817,
        "params": {
            "clawback": "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAY5HFKQ",
            "creator": "TALE7TBFEOCK4Q4M4QLYTPYTNTA2L7MYDY7IBYK3UOO4TIXH2YJBBS3EWU",
            "decimals": 0,
            "default-frozen": false,
            "freeze": "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAY5HFKQ",
            "manager": "TALE7TBFEOCK4Q4M4QLYTPYTNTA2L7MYDY7IBYK3UOO4TIXH2YJBBS3EWU",
            "name": "Protagonist C2α - Ancient",
            "name-b64": "UHJvdGFnb25pc3QgQzLOsSAtIEFuY2llbnQ=",
            "reserve": "TALE7TBFEOCK4Q4M4QLYTPYTNTA2L7MYDY7IBYK3UOO4TIXH2YJBBS3EWU",
            "total": 1,
            "unit-name": "🐰",
            "unit-name-b64": "8J+QsA=="
        }
    },
    "current-round": 20388498
}

The freeze and clawback addresses were set to None on creation. As a result, calling indexer.asset_info yields the following (truncated for brevity).

Asset {
    params: AssetParams {
        clawback: Some(AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAY5HFKQ),
        ..
    }
}

Apparently, attempting to create an asset with the AAAA... address yields an error.... which means that the zero address actually means "missing address".

Expected behavior I was expecting:

Asset {
    params: AssetParams {
        clawback: None,
        ..
    }
}

Additional context I personally think that algonaut should understand these zero addresses and parse them as "None"s. This obviously introduces complexities and potential bug in the future if existing behavior ever changes.

The other option is to leave them alone, but get rid of the Option from the various addresses. I don't like this.

Regardless of what happens, this behavior is mildly janky and should be documented.

ivnsch commented 2 years ago

Hmm that seems mainly an API issue. The node's API has this policy (which I dislike) where it omits / expects us to omit values that are considered "zero", which includes arrays with only zeros, or structs with values that are only zeros (this leads to us having to implement "smart" deserialization in some places, like here: https://github.com/manuelmauro/algonaut/blob/0d93ce108d66a593ccc0840c0e9bca6dee795e3c/algonaut_transaction/src/api_model.rs#L395-L415)

The indexer sending these zero addresses is also an inconsistency with that. Probably since it uses strings, their serializer doesn't recognize them as a "zero value" and sends it.

I'd be inclined to raise the issue in their repo https://github.com/algorand/go-algorand. But not against patching it in the SDK.

Edit: this is the indexer's repository: https://github.com/algorand/indexer (ignoring the node's zero values policy, this only affects the indexer).

AlterionX commented 2 years ago

We should probably do both, then. (As in, both patching it in the SDK & raising an issue.)

Another thing to think about is if we need different deserialization protocols for algod/indexer/kmd.

Does anyone know if I can just plop this into the indexer issues?

AlterionX commented 2 years ago

Possibly relates to #98?

ivnsch commented 2 years ago

We should probably do both, then. (As in, both patching it in the SDK & raising an issue.)

Agreed.

Another thing to think about is if we need different deserialization protocols for algod/indexer/kmd.

Does anyone know if I can just plop this into the indexer issues?

If with protocols you mean JSON / MessagePack, For algod/indexer/kmd only JSON should be enough - MessagePack is used only to send transactions and TEAL. Currently Transaction can however be deserialized only from MessagePack. We've e.g. this place, where Transaction (which is included in the pending_transactions response) is not defined, maybe because of insecurities about how to handle the JSON deserialization. If we adjust the ApiTransaction visitors to deserialize from both MsgPack and JSON we might be able to reuse ApiTransaction / Transaction for this endpoint (and possibly others that return transactions - would have to check whether they share the same fields). The Transaction deserialization from MsgPack, while not required, should be preserved just for symmetry with the serialization, which you might use in contexts different to the Algorand APIs.

Possibly relates to https://github.com/manuelmauro/algonaut/issues/98?

It's a bit wider, but yeah it relates.

ivnsch commented 2 years ago

Also relates to https://github.com/manuelmauro/algonaut/issues/106