GetCompactFieldDecoder decodes the wrong type

Eduard-Voiculescu commented 3 months ago

Quick question here: while checking the conversion of compact types, I noticed this.

Why is it that when we convert a compact type, we will recursively call the getCompactFieldDecoder instead of calling getFieldDecoder and letting it handle the child's fieldDecoder. This is pretty much what has been done for all the other cases (check array, tuple, etc.)

Even this will call getFieldDecoder.

I would expect that this snipped of code:

for _, compactCompositeField := range compactCompositeFields {
    compactCompositeFieldType, ok := meta.AsMetadataV14.EfficientLookup[compactCompositeField.Type.Int64()]

    if !ok {
        return nil, ErrCompactCompositeFieldTypeNotFound
    }

    compactFieldName := getFullFieldName(compactCompositeField, compactCompositeFieldType)

        // CHECK HERE: f.getCompactFieldDecoder should be changed to f.getFieldDecoder
    compactCompositeDecoder, err := f.getCompactFieldDecoder(meta, compactFieldName, compactCompositeFieldType.Def)

    if err != nil {
        return nil, ErrCompactCompositeFieldDecoderRetrieval.Wrap(err)
    }

    compositeDecoder.Fields = append(compositeDecoder.Fields, &Field{
        Name:         compactFieldName,
        FieldDecoder: compactCompositeDecoder,
        LookupIndex:  compactCompositeField.Type.Int64(),
    })
}

If not, what am I missing here or what am I not understanding correctly?

cdamian commented 3 months ago

@Eduard-Voiculescu that is because compact types cannot be parsed normally, using the getFieldDecoder. There are "special" rules when it comes to:

compact primitives - where we always return UCompact;
compact composites - we expect a composite type with compact fields here;
compact tuples - we expect a tuple of the compact types mentioned above;

These are the only compact types that we stumbled upon on centrifuge and polkadot when this was initially implemented, I would also expect that other similar compact types might come up at some point.

Is there a particular issue that you are trying to debug here/any new type that we missed?

Eduard-Voiculescu commented 3 months ago

Yes, I have stumbled upon some issues.

So here is the issues that I found (using metadata v14):

When parsing the type 4:

"4": {
    "Path": null,
    "Params": null,
    "Def": {
      "IsComposite": false,
      "Composite": {
        "Fields": null
      },
      "IsVariant": false,
      "Variant": {
        "Variants": null
      },
      "IsSequence": false,
      "Sequence": {
        "Type": 0
      },
      "IsArray": false,
      "Array": {
        "Len": 0,
        "Type": 0
      },
      "IsTuple": false,
      "Tuple": null,
      "IsPrimitive": true,
      "Primitive": {
        "Si0TypeDefPrimitive": 5
      },
      "IsCompact": false,
      "Compact": {
        "Type": 0
      },
      "IsBitSequence": false,
      "BitSequence": {
        "BitStoreType": 0,
        "BitOrderType": 0
      },
      "IsHistoricMetaCompat": false,
      "HistoricMetaCompat": ""
    },
    "Docs": null
  },

This one here will return a types.U32, which is perfect and expected.

However, when parsing the type 51:

"51": {
        "Path": [
          "sp_arithmetic",
          "per_things",
          "Perbill"
        ],
        "Params": null,
        "Def": {
          "IsComposite": true,
          "Composite": {
            "Fields": [
              {
                "HasName": false,
                "Name": "",
                "Type": 4,
                "HasTypeName": true,
                "TypeName": "u32",
                "Docs": null
              }
            ]
          },
          "IsVariant": false,
          "Variant": {
            "Variants": null
          },
          "IsSequence": false,
          "Sequence": {
            "Type": 0
          },
          "IsArray": false,
          "Array": {
            "Len": 0,
            "Type": 0
          },
          "IsTuple": false,
          "Tuple": null,
          "IsPrimitive": false,
          "Primitive": {
            "Si0TypeDefPrimitive": 0
          },
          "IsCompact": false,
          "Compact": {
            "Type": 0
          },
          "IsBitSequence": false,
          "BitSequence": {
            "BitStoreType": 0,
            "BitOrderType": 0
          },
          "IsHistoricMetaCompat": false,
          "HistoricMetaCompat": ""
        },
        "Docs": null
      },

This one will return a types.Composite which contains a types.UCompact when it reaches the type "4". But this doesn't respect the Metadata v14 spec, which expects a types.U32 for the type 4 when parsing 51.

Eduard-Voiculescu commented 3 months ago

However, I am sure you have digged a lot and much much more than I have, so I am curious if you saw a byte to something that was off in the case of a Composite?

cdamian commented 3 months ago

OK, then something is definitely off there and we need to take a closer look.

Can you please provide me with the URI of the chain in question? Also, if possible, a block number where an event/extrinsic with this type can be found?

Eduard-Voiculescu commented 3 months ago

Yes of course.

Chain: Vara-mainnet Block reference: 0xdacec8ff307e047ff4dfa08a1a59dea0326b12902658e52de310a4214c04686c Extrinsic: Staking -> Validate Call

The metadata: https://docs.blastapi.io/blast-documentation/apis-documentation/core-api/vara/state/state_getMetadata

I also created this small go program if you want to use it:

package main

import (
    "encoding/json"
    "log"
    "os"

    gsrpc "github.com/centrifuge/go-substrate-rpc-client/v4"
)

func main() {
    rpcEndpoint := "https://vara-mainnet.public.blastapi.io"
    api, err := gsrpc.NewSubstrateAPI(rpcEndpoint)
    if err != nil {
        log.Fatal("Error creating API instance")
    }

    metadata, err := api.RPC.State.GetMetadataLatest()
    if err != nil {
        log.Fatalf("Failed to get metadata: %v", err)
    }

    b, err := json.MarshalIndent(metadata, "", "  ")
    if err != nil {
        log.Fatalf("Failed to marshal metadata: %v", err)
    }

    err = os.WriteFile("metadata.json", b, 0644)
    if err != nil {
        log.Fatalf("Failed to write metadata: %v", err)
    }
}

cdamian commented 3 months ago

Thanks for sharing, I managed to do some digging and here is what I found:

The prefs: PalletStakingValidatorPrefs commission field is a compact Perbill according to - https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fvara-mainnet.public.blastapi.io#/explorer/query/0xdacec8ff307e047ff4dfa08a1a59dea0326b12902658e52de310a4214c04686c

I also checked the call registry that is generated and that seems to match the definition found there:

Field type info for PalletStakingValidatorPrefs:

Field type info for the commission field of PalletStakingValidatorPrefs:

Coming back to your original point, field number 51 - Perbill is not the one being used when decoding the commission, instead field number 53 is used since that is a Compact Perbill, as expected.

Here's a gist that has the script I used to check this - https://gist.github.com/cdamian/4d7783d953cca061a30f12db4aab8344

Eduard-Voiculescu commented 2 months ago

Hey @cdamian, sorry I never got back to you, but I was able to figure out the issue thanks to your help.

Appreciate it!

centrifuge / go-substrate-rpc-client

GetCompactFieldDecoder decodes the wrong type #395