This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Propose bumping StringLimit to 128 bytes #516
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The StringLimit of 50 bytes is too short for useful hashes of different protocols: - When using IPFS, the hash is 46 bytes. With a protocol prefix and type prefix (IPFS vs IPNS) it becomes: `ipfs://ipfs/QmPK1s3pNYLi9ERiq3BDxKa4XosgWwFRQUydHUtz4YgpqB` or 59 bytes. - On arweave, the hash is 43 bytes. Prefixed with just `arweave://` bumps it up to 53: `arweave://BNttzDav3jHVnNiV7nYbQv-GY0HQ-4XXsdkE5K9ylHQ`. - On Sia, with 46 char hashes, adding `sia://` as the protocol to use will bump it past 50: `sia://GACjmEWXmYF1N3Rc-PyjN304-8M0zOXHYzAXY9222xkGhA` (52) As such, we feel that 64 bytes is a reasonable minimum for the `StringLimit`.
Swader
changed the title
|
I'm fine with this,
|
This is what you should do. Please make a new constant specific for this. |
shawntabrizi
approved these changes
Jul 1, 2021
joepetrowski
approved these changes
Jul 2, 2021
apopiak
added
the
T7-system_parachains
slumber
pushed a commit
that referenced
this pull request
Sep 1, 2021
* Propose bumping StringLimit to 64 bytes The StringLimit of 50 bytes is too short for useful hashes of different protocols: - When using IPFS, the hash is 46 bytes. With a protocol prefix and type prefix (IPFS vs IPNS) it becomes: `ipfs://ipfs/QmPK1s3pNYLi9ERiq3BDxKa4XosgWwFRQUydHUtz4YgpqB` or 59 bytes. - On arweave, the hash is 43 bytes. Prefixed with just `arweave://` bumps it up to 53: `arweave://BNttzDav3jHVnNiV7nYbQv-GY0HQ-4XXsdkE5K9ylHQ`. - On Sia, with 46 char hashes, adding `sia://` as the protocol to use will bump it past 50: `sia://GACjmEWXmYF1N3Rc-PyjN304-8M0zOXHYzAXY9222xkGhA` (52) As such, we feel that 64 bytes is a reasonable minimum for the `StringLimit`. * Update lib.rs * Add different const for `UniquesStringLimit` Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The StringLimit of 50 bytes is too short for useful hashes of different protocols:
ipfs://ipfs/QmPK1s3pNYLi9ERiq3BDxKa4XosgWwFRQUydHUtz4YgpqBor 59 bytes. With CIDv1, the hashes are even longer, and with the prefixes they become 71 chars long:ipfs://ipfs/bafybeierhgbz4zp2x2u67urqrgfnrnlukciupzenpqpipiz5nwtq7uxpx4.arweave://bumps it up to 53:arweave://BNttzDav3jHVnNiV7nYbQv-GY0HQ-4XXsdkE5K9ylHQ.sia://as the protocol to use will bump it past 50:sia://GACjmEWXmYF1N3Rc-PyjN304-8M0zOXHYzAXY9222xkGhA(52)To allow for static URLs which are typically longer than 50 bytes, and also to cover all of the above hashes and protocols, we feel that 128 bytes is a reasonable minimum for the
StringLimit.