BoundedString to BoundedVec #622
Conversation
Klapeyron
force-pushed
the
ETCM-9674_small_refactoring
branch
from
a01972a to
8d5408e
Compare
| @@ -461,13 +461,15 @@ impl AsCardanoSPO for BlockAuthor { | |||
| } | |||
| } | |||
|
|
|||
| pub const MAX_METADATA_URL_LENGTH: u32 = 512; | |||
| parameter_types! { | |||
| pub const MaxMetadataUrlLength: u32 = 512; | |||
There was a problem hiding this comment.
looks quite long, should we short it?
|
|
||
| #[derive( | ||
| Clone, Debug, MaxEncodedLen, Encode, Decode, Serialize, Deserialize, PartialEq, Eq, TypeInfo, | ||
| )] | ||
| pub struct BlockProducerMetadataType { | ||
| pub url: BoundedString<MAX_METADATA_URL_LENGTH>, | ||
| pub url: BoundedVec<u8, MaxMetadataUrlLength>, |
There was a problem hiding this comment.
How is this change reflected in RPC?
I would like to see "before" and "after" before approving such a change.
There was a problem hiding this comment.
I think it is hard to answer clearly that one, it actually depends on what will be the source of data. If type will be created from runtime (i.e. Decode trait will be involved), then in theory it may contain non utf-8 characters, so further serialization of this type with serde::Serialize may cause permanent error.
Other scenario will be if type will be created with serde::Deserialize, then we will have validation being involved (utf-8 check), but later there are no guarantees that data will remain utf-8 (code before changes).
From the user perspective providing of the data should stay the same, user will provide some arbitrary blob of bytes that may or may not decode to string, point here is to not make any assumptions regarding what is inside (due we not validate either utf-8 data or url format).
After the changes serialization of the data back to readable format may change, proposed changes will try to save them in bytes format, without attempt to provide "readable" utf-8 form, due underlying data may be invalid
Klapeyron
force-pushed
the
ETCM-9674_small_refactoring
branch
from
aec15f1 to
7d55434
Compare
Klapeyron
force-pushed
the
ETCM-9674_small_refactoring
branch
from
98215ec to
adc3de0
Compare
Could you add a bit context in the description like the rationale/benefit of the refactoring? I might learn something :) |
|
I put some details regarding changes in this PR in associated Jira item |
|
|
||
| #[derive( | ||
| Clone, Debug, MaxEncodedLen, Encode, Decode, Serialize, Deserialize, PartialEq, Eq, TypeInfo, | ||
| )] | ||
| pub struct BlockProducerMetadataType { | ||
| pub url: BoundedString<MAX_METADATA_URL_LENGTH>, |
There was a problem hiding this comment.
This is our own reference runtime implementation, not some code that anyone other than us is forced to use. We can make any assumptions that are convenient for us to do here. I went with a string-like type, because that's easy and intuitive for us to work with.
|
If you wish to keep the changes in this form, I will consider the matter closed |
Description
Proposition to replace
BoundedStringwithBoundedVec, in order to not make assumptions of content of underlying data provided by the user.If I understand correctly the execution path, it probably is the following:
Checklist
changelog.mdfor affected crate