blocto / solana-go-sdk

Solana Golang SDK
https://blocto.github.io/solana-go-sdk/
MIT License
366 stars 95 forks source link

Allow deserialization of corrupt on-chain metadata. #81

Closed omarkilani closed 2 years ago

omarkilani commented 2 years ago

Unfortunately, there are many mints on-chain with corrupt metadata due to a bug in the Metaplex token metadata program.

One such example is:

5UJvyhCsum71CK9ERrH8hcLw6aWzh4SjwWXToquWdWR3

Which fails to MetadataDeserialize with this error:

failed to deserialize data, err: expected bool is 0 or 1, got 122

As there's corrupt data after the EditionNonce.

With this patch, we attempt to deserialize the data into the normal Metadata struct.

If that fails, we try to deserialize it into the MetadataPreV11 struct.

If that succeeds, we copy the values from the MetadataPreV11 struct into the Metadata struct up to and including EditionNonce.

(Note: I didn't embed the MetadataPreV11 into the Metadata struct as it makes initializating the Metadata struct too annoying.)

C.f.:

https://github.com/samuelvanderwaal/metaboss/issues/121

https://github.com/metaplex-foundation/metaplex-program-library/pull/407

https://github.com/metaplex-foundation/metaplex-program-library/blob/master/token-metadata/program/src/deser.rs#L12

omarkilani commented 2 years ago

@yihau I don't know if there's a more elegant way to do this, but this would be very useful to have in the upstream library.

yihau commented 2 years ago

hey! Thank you! Interesting... IIRC metaplex follows borsh in the very beginning. how do those NFTs have invalid data 🤔

tbh, I prefer to use the idea you pasted in(https://github.com/metaplex-foundation/metaplex-program-library/blob/master/token-metadata/program/src/deser.rs#L12) cuz I think it can make this function have more compatibilities. the situation is like the token metadata account keeps changing, if we use the function you wrote. the future look will be more like

if datav13, err := deserializeTokenMetadatav13
// handle error and wrap
        if datav12, err := deserializeTokenMetadatav12
        // handle error and wrap
            if datav11, err := deserializeTokenMetadatav11
            // return error here

if we use the link's idea, we just need to add a new column parsing logic at the end of the function.

I have made a prototype like https://github.com/portto/solana-go-sdk/blob/fix-metadata-deserialize/program/metaplex/tokenmeta/state.go#L104 the test https://github.com/portto/solana-go-sdk/blob/fix-metadata-deserialize/program/metaplex/tokenmeta/state_test.go#L127-L151

I think you use this SDK a lot, would like to know wdyt?

omarkilani commented 2 years ago

Hi @yihau,

The corruption is on-chain because the old versions of the metaq* program did not 0 out the data after borsh serialisation when the creator vector was resized down (e.g. 2 creators -> 1 creator). No one realised this until the on-chain program tried to deserialise the corrupt data into the new 1.1 struct.

It shouldn't happen again, so it's only really < V1.1 and >= V1.1.

I think your way makes sense too, I just didn't want to re-invent borsh.

yihau commented 2 years ago

gm @omarkilani!

ah. okay. Thank you for the info! if branches are only < v1.1 and >= v1.1, I think your implementation is more make sense.

before merge it, just some nitpick

  1. maybe we don't export MetadataPreV11 struct, I think it should only live in this scope atm
  2. could you pick the test (https://github.com/portto/solana-go-sdk/blob/fix-metadata-deserialize/program/metaplex/tokenmeta/state_test.go#L127-L151) into your PR
omarkilani commented 2 years ago

@yihau ✅

yihau commented 2 years ago

Thank you! 😎