Improve type safety by wrapping value types in structs

[hackaugusto]

Currently uint256 is used for timeouts, chain_id, token amounts, channel identifiers, nonces, block_numbers, and expirations. On most cases these are completely incompatible and mixing the values is a bug, and on some cases care is required to convert from one type to another (timeouts -> expirations).

To avoid errors and have a type with semantic meaning I propose the use of the following pattern to define types:

struct ChannelId { uint256 id; }
struct Channel {ChannelId id;}

instead of using the value type directly as it's currently done:

struct Channel {uint256 id;}

I checked the bytecode generated for code similar to the one above with 0.4.24+commit.e67f0147.Emscripten.clang and there is no negative impact on the generated code (without optimizations)

Edit: Updated the code snippet because structs cannot be used as mapping keys

[LefterisJP]

and there is no negative impact on the generated code

Do you mean it looks identical? iow has no extra opcodes?

[hackaugusto]

Do you mean it looks identical? iow has no extra opcodes?

yes

[LefterisJP]

If so this seems like a nice approach. We do similar stuff (structs with a single member) to get compiler time type-checking in C.

[hackaugusto]

We do similar stuff (structs with a single member) to get compiler time type-checking in C.

Yep, same idea

[loredanacirstea]

Did you only check for the code above or did you also check with code that actually uses the ChannelId? I expect additional gas (probably minimal) from computing the pointer to the actual id value.

Do you know if web3.py already supports struct as input? I know there were some problems implementing this at some point.

[hackaugusto]

Did you only check for the code above or did you also check with code that actually uses the ChannelId?

I did not check with our current code.

I expect additional gas (probably minimal) from computing the pointer to the actual id value.

I had some guesses written here, but instead of guessing we should just do a test and see if it's the same.

Do you know if web3.py already supports struct as input? I know there were some problems implementing this at some point.

I don't know for sure, the only issue open on the web3.py repo is about improving the current return value. I believe that structs as input is part of the new ABI, which still experimental, so I would not expect inputs to work right now.

That is something that we can fix (contributing code or reporting issues). If there are issues we could either wait until everything is working or use more descriptive types for the internal data structures with explicit casting for the inputs. I think the type safety is worth the additional lines of code/trouble.

[loredanacirstea]

@hackaugusto , please provide the entire code of what you have tested. For me, it does not compile and solidity/issues/599 does not seem closed (even though the docs say it is possible for a while now)

map is not correct code, so I assume the above was pseudocode.

Otherwise, we can test later and see if this has benefits for the contracts.

[loredanacirstea]

I see why

struct ChannelId { uint256 id; }
struct Channel {ChannelId id;}

is useful and can protect from unintended operations on two different types (e.g. channel_identifier + settlement_timeout).

Implementing this at the moment would also mean changing the contracts structure, because - as seen earlier, mappings cannot have struct as keys - at least not yet.

So, what do you propose? Wait? Restructure?

[LefterisJP]

So, what do you propose? Wait? Restructure?

@loredanacirstea definitely wait. This can happen at any point later in the future.