[pallet-revive] Move to_account_id host function to System pre-compile

[cmichi]

Part of closing #8572.

cc @athei @pgherveou

[cmichi]

Is the assumption of AccountId32 correct or should we rather use bytes memory here? I'm not sure if pallet-revive supports generic AccountIds?

[pgherveou]

in theory it could be anything,
but most likely if this is not an accountId32 it's an H160 and you don't need this precompile at all.

Maybe just call it toAccountId32 to be explicit that this is intended for when the accountId is an accountId32, and mention that this revert if that's not the case

[athei]

It doesn't revert though. We are using T::AccountId.encode() here. In case that is everything else than AccountId32 we just return undecodeable garbage. I guess Solidity will just panic in this case. But it should be documented to not call this function on a native H160 chain.

[cmichi]

Okay, so my understanding from what you both wrote:

• Since you are using T::AccountId.encode(), which could be anything, the function must return bytes memory. That's also why the function name stays with toAccountId().
• Calling this function on a chain with type AccountId = H160 does not make sense, as it would just return the address that it was called with. This is what we want to document.

Correct?

@athei What do you mean with this?

In case that is everything else than AccountId32 we just return undecodeable garbage.

Why would it be undecodable garbage if the chain uses anything else than type AccountId = AccountId32?

[cmichi]

I've updated the PR with the assumptions made above.

[pgherveou]

It doesn't revert though. We are using T::AccountId.encode()

I am fine switching it to the current returns (bytes memory account_id)
I was thinking that it should revert if we do a try_into and return a 32 byte array.
keeping a static return type with the original returns (bytes32 account_id) would be more efficient thus my suggestion above.

[athei]

Why would it be undecodable garbage if the chain uses anything else than type AccountId = AccountId32?

Because the data type says 32bytes but you return a different number of bytes. Solidity will fail during decoding.

[davidsemakula]

Because the data type says 32bytes but you return a different number of bytes. Solidity will fail during decoding.

FYI, for Solidity's abi.decode, this is only true if you return less bytes than the target type (i.e. less than 32 in this example), the more bytes than expected case will decode fine (the excess bytes are essentially ignored)

[athei]

It doesn't revert though. We are using T::AccountId.encode() here. In case that is everything else than AccountId32 we just return undecodeable garbage. I guess Solidity will just panic in this case. But it should be documented to not call this function on a native H160 chain.

[athei]

Looks good. But let's hold off merging until #9487 is in.

[paritytech-release-backport-bot]

Successfully created backport PR for unstable2507:

• [unstable2507] Backport #9455 #9503