Update Type Definitions to latest PolkadotJS#34
Conversation
- Remove TYPES_META (it's not used, and not in the latest polkadot.js definitions) - Revise Generic Parsing regex
Co-authored-by: David <dvdplm@gmail.com>
…h/desub into insipx/update-type-definitions
Co-authored-by: David <dvdplm@gmail.com>
|
|
||
| /// Decodes old address pre-refactor (https://github.com/paritytech/substrate/pull/7380) | ||
| /// and converts it to a MultiAddress, where "old" here means anything before v0.8.26 or 26/2026/46 on polkadot/kusama/westend respectively. | ||
| fn decode_old_address(data: &[u8], cursor: &mut usize) -> Result<substrate_types::Address, Error> { |
There was a problem hiding this comment.
AFAIU there's nothing checks that cursor and cursor + 1 are valid indexes into data slice but maybe checked somewhere else?
It may panic then.
There was a problem hiding this comment.
Yeah,
This is part of the larger issue that the function decode_single doesn't do any bounds checking on the cursor at all, and panics if anything goes wrong. I'd rather address that in it's own PR though, since it might be a bit of a larger/medium sized refactor so I opened #35 to track that
core/src/regex.rs
Outdated
| } | ||
|
|
||
| pub fn rust_bit_size() -> Regex { | ||
| Regex::new(r"^(Int|UInt)<([\w\d]+), ?([\w\d]+)>").expect("Regex expression should be infallible; qed") |
There was a problem hiding this comment.
I guess this regex is for Int<size, type>, I'm not a regex export but...
I'm surprised that [\d\w]+ is used for the size, it would match beef999bibimbap for example. Thus, if size is in decimal just use \d+ then.
Why , ?([\w\d]+) it would only work for Int<999,bar> and Int<999, bar> but not for arbitrary number of spaces :)
I suggest , *(\w+) instead.
There was a problem hiding this comment.
I don't think the capture groups are necessary for the type params, i.e. this should be fine: (Int|UInt)<[\w\d]+, *[\w\d]+> and like niklas I'm not sure I understand what the input to this looks like: can both params contain both letters and number? A test that exercise this case would be good.
There was a problem hiding this comment.
Here's a sample of some of the input from polkadot.js:
"Fixed64": "Int<64, Fixed64>",
"FixedI64": "Int<64, FixedI64>",
"FixedU64": "UInt<64, FixedU64>",
"Fixed128": "Int<128, Fixed128>",
"FixedI128": "Int<128, FixedI128>",
"FixedU128": "UInt<128, FixedU128>",
"I32F32": "Int<64, I32F32>",
"U32F32": "UInt<64, U32F32>",
"PerU16": "UInt<16, PerU16>",
"Perbill": "UInt<32, Perbill>",
"Percent": "UInt<8, Percent>",
"Permill": "UInt<32, Permill>",
"Perquintill": "UInt<64, Perquintill>",
"Balance": "UInt<128, Balance>",So the second type parameter is unnecessary, since it's already a key in our de-serialized JSON, but the first type parameter is needed to delegate the size of the type we are dealing with. This also leads me to believe that the first type parameter will always be a digit and the second just a repeat of the JSON key (I haven't found anything in the definitions that contradicts this). I'm not 100% on what the rationale of having the type repeat is in Polkadot.JS, but in the PR's I found like here they use the Type<BitLength> in that style.
I agree with @niklasad1 , though, arbitrary number of spaces is definitely preferable, fixes for these/and above incoming
There was a problem hiding this comment.
All right, I see
We could do (Int|UInt)<[\d]+(, *[\w\d]+)?> instead to make the second type parameter optional then I guess
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
…h/desub into insipx/update-type-definitions
Latest PolkadotJS includes two new types
UInt<size, type>andInt<size, type>. There were a few ways to handle this:UIntandIntin the generic matching code and redirect it to instead decode this as a primitive integerI went with the second option as it seem the most flexible with future changes, and requires less code added to
decoder.rs(which could be confusing as a generic redirects to a primitive)Includes changes to update to latest master from #33