search

polkadot-fellows / runtimes

The various runtimes which make up the core subsystems of networks for which the Fellowship is represented.
GNU General Public License v3.0
134 stars 85 forks source link

Field `image` on people chain is unusable #418

Open leonardocustodio opened 1 month ago

leonardocustodio commented 1 month ago

Hello guys,

A field called image was added to setIdentity at people chains. The thing is this field takes a Data input and the Raw value can be at most 32 bytes.

This makes the field unusable for two reasons:

  1. We cannot upload any images (I know this is not the goal maybe here)
  2. The most important, we can't even fit an IPFS URL in 32 bytes.....

We should probably allow this field to be set with a bigger string.

peetzweg commented 1 month ago

My guess is, it is meant for storing just the hash of the image.

Than you can use the hash to fetch the actual image from somewhere else, potentially IPFS or any other sources as you can verify the image by the hash.

For IPFS you would construct a CID from the hash and a codec to look it up.

leonardocustodio commented 1 month ago

Many websites that people use to upload ipfs files, as not many people know much about ipfs, generates a hash for a folder and the image is inside a folder.

If we have to combine the folder hash plus the image name, it will not fit.

In my opinion this field should take just a simple Bytes input. Maybe we can limit the size but definetly needs to be bigger.

I believe the same thing should be done for the Legal Name. Usually people expect the Legal Name to be the complete name, either for a person or a company.

My wife's full name doesn't fit. My company full name doesn't fit.

Considering you set the field as Raw and as pretty much all system that reads the identity that we have right now read those fields as just simple hex encoded strings.

joepetrowski commented 1 month ago

The 32 byte limit is something hardcoded into the Identity pallet (here). As @peetzweg said, the hash data types assume that a UI has access to the preimage.

IMO it would be fine to increase this to something like 64 so that it can accommodate a link that includes a 32 byte hash. That should solve the vast majority of company/personal names as well (and longer names could be looked up from a link that does fit in that size).

Making the value configurable would be nice but would be a breaking change to the pallet config. Would suggest changing the limit directly to avoid breaking changes, or to incorporate the change into the next set of breaking changes (cc @georgepisaltu).

leonardocustodio commented 1 month ago

I believe that changing the Data.Raw type will break way more things than using a new type.

Many scale codec libs have hard coded the Data.Raw type, here is one example: https://github.com/polkascan/py-scale-codec/blob/f676638998ce612d9f0b38b135f77a95eacd51ce/scalecodec/types.py#L1334-L1415

Also, there are people using the Data.Raw on other pallets/extrinsics. To me, just changing the type of IdentityInfo with a change in those two fields is a better approach, as we would be sure the "breaking" is limited to Identity only.

georgepisaltu commented 3 weeks ago

The image field is not really for raw data, it's for hashes. It's up to the UI provider to determine how to store the images, the field is there so that you can verify the information against what is on chain.

I believe the same thing should be done for the Legal Name. Usually people expect the Legal Name to be the complete name, either for a person or a company. My wife's full name doesn't fit. My company full name doesn't fit.

If the data doesn't fit in 32 bytes, you should use a hash type instead of Data::Raw and store said data elsewhere, while maintaining the ability to verify it against data that is on chain. The point of storing this IdentityInformation on chain is to be able to prove an identity, not store all data associated with an identity.

In my opinion this field should take just a simple Bytes input. Maybe we can limit the size but definetly needs to be bigger.

I don't agree with this. Right now the Data is just 32 bytes with different interpretations. If we want to allow for variable sizes, the current structures of both Data and IdentityInformation (basically a fixed size list of these Data) don't really make sense anymore. I don't think this image/IPFS link in particular is worth the extra complexity we would take on, along with a big migration.

To address your issue, the best option is to follow the advice in @peetzweg 's comment. The second best option IMO is to make an RFC proposing the addition of 2 new fields in IdentityInformation, something like ipfs_hi and ipfs_lo, which used together would be an IPFS CID. If you see value in this IPFS use case, I encourage you to get involved and try the RFC route, maybe there are lots of people out there wanting this and it gains traction.