Change offset parameter from u32 to i32

[qdm12]

• WASM does not support u32, see
  https://www.w3.org/TR/wasm-core-1/#value-types%E2%91%A0
• Affects ext_default_child_storage_read_version_1
• Affects ext_storage_read_version_1

❓ Although it this offset is meant to be u32, this is a bit problematic since in our implementation it's really cut down to the max i32 (due to the function signature) which is lower than max u32. Should we document to treat the 32 bits as uint32 within the function body? 🤔

[lamafab]

Thanks. While it is true that Wasm itself does not directly support unsigned integers, it's still important for the implementation to interpret it as an unsigned integer. I think we should convert the function definition to i32 (as you did) but make it clear in the argument listing that it's u32, thoughts?

Btw, @FlorianFranzen this might be a good idea for your testsuite: what happens when you use u32::MAX values on alternative Host implementations? This could also be fuzzed.

[qdm12]

Rebased the branch, and changed it to an u32 integer (typed as i32 due to wasm types)

[bkchr]

The types i32 and i64 classify 32 and 64 bit integers, respectively. Integers are not inherently signed or unsigned, their interpretation is determined by individual operations.

I would keep it as u32 because that is how you need to interpret the bits.

[qdm12]

That makes sense, what I'm trying to do is highlight the need to NOT take it as i32.
In our implementation we simply overlooked the u32 because wasm only supports i32. So we did not give special care to handling bigger than max i32 values. Maybe we can re-phrase specific uses of u32 so it highlights it should be treated as u32 and not just i32.

[bkchr]

In our implementation we simply overlooked the u32 because wasm only supports i32

IMO sounds to me like you didn't read the spec 🙈 Maybe we can add in some kind of initial description that unsigned values are possible.

[qdm12]

It still think the type in the signature should be i32 due to wasm, but it should be mentioned in the description it should be interpreted as u32. I would also rather have a local description (in those two functions affected) than a global one somewhere at the top, which might be overlooked. If this would affect a lot of host functions, however this could make more sense too.

I understand from a validity standpoint the spec is valid, but from a human reader/implementer perspective that would help 😉

[bkchr]

I will not do the call here on what is right, however what should be considered is that we have a lot of u32/u64 in function signatures. So, I would just explain this once in some introduction to the host functions. What would you prefer @lamafab?

[qdm12]

Good point, I forgot about u64 as well (let me check our implementation 😄!)
On the other hand, we have 2 signatures using u32 and 2 signatures using u64, so it's not that many functions either.

If we have this setup in the test suite for max u32 values, that would do it as well and we might not need to change the spec at all, since this would automatically enforce correct behavior right.

[lamafab]

I think this is fine, thx!

[lamafab]

@qdm12

and 2 signatures using u64

I assume ext_offchain_timestamp_version_1 and ext_offchain_sleep_until_version_1? I'll update those, too.

EDIT: done -> 9752117

[qdm12]

My bad I forgot those for some reason. 👍
Should I create an issue to test those for max u32/u64 on the test suite repo?