Currently, messages are hashed as keccak256(prefix || digest), prefix being a message name used to avoid collisions with other messages - both in terms of internal messages and also other protocols'.
However, if we are to be completely resistant to collisions, separate deployments should also be considered. As EIP712 has specific chain ID and verifier contract specifications, something similar, though not exactly the same, is needed here. Otherwise, we can have collisions with different deployments of the protocol. Of course, that would be avoidable in the current state by changing the message names on the other deployment and, considering BLS keys should differ between them, this is not an issue at the current stage. Also, the collision wouldn't be impactful.
Either way, this is something that should be changed soon because it affects the messaging protocol formats. So, the sooner the better.
New Behavior
We can't properly use something as EIP712, as already discussed in #104, because our messages are actually meant to be verified cross-chain by different contracts. At the same time, changing constant message prefixes per deployment is not that interesting of a solution. It could be if we had a Solidity preprocessor though.
What we can do instead is introducing a deployment-specific member to our prefix. In this case, it's better to use a domain separator already. So, with this we now have a protocolVersion member which is set up in the SFFLTaskManager contract and SFFLRegistryRollup contracts as immutable (can be changed in upgrades, which would already be needed anyway in case of a messaging protocol change) and we use a type hash domain separator for hashing messages through SFFLDomain(bytes32 name,bytes32 protocolVersion). The only detail here is protocolVersion MUST be different from deployment to deployment. So, what could be suggested is simply keccak256(vX.Y.Z-<network>-<nonce>), but it doesn't need to follow a specific pattern.
Breaking Changes
This is a breaking change and requires coordination for upgrades. The reason for that is nodes, as contracts, hash messages, and these hash functions must be the same. We could make a compatibility layer, but it'd require operators to track contract upgrades, so it's some added complexity.
Current Behavior
Currently, messages are hashed as
keccak256(prefix || digest),prefixbeing a message name used to avoid collisions with other messages - both in terms of internal messages and also other protocols'.However, if we are to be completely resistant to collisions, separate deployments should also be considered. As EIP712 has specific chain ID and verifier contract specifications, something similar, though not exactly the same, is needed here. Otherwise, we can have collisions with different deployments of the protocol. Of course, that would be avoidable in the current state by changing the message names on the other deployment and, considering BLS keys should differ between them, this is not an issue at the current stage. Also, the collision wouldn't be impactful.
Either way, this is something that should be changed soon because it affects the messaging protocol formats. So, the sooner the better.
New Behavior
We can't properly use something as EIP712, as already discussed in #104, because our messages are actually meant to be verified cross-chain by different contracts. At the same time, changing constant message prefixes per deployment is not that interesting of a solution. It could be if we had a Solidity preprocessor though.
What we can do instead is introducing a deployment-specific member to our prefix. In this case, it's better to use a domain separator already. So, with this we now have a
protocolVersionmember which is set up in theSFFLTaskManagercontract andSFFLRegistryRollupcontracts as immutable (can be changed in upgrades, which would already be needed anyway in case of a messaging protocol change) and we use a type hash domain separator for hashing messages throughSFFLDomain(bytes32 name,bytes32 protocolVersion). The only detail here isprotocolVersionMUST be different from deployment to deployment. So, what could be suggested is simplykeccak256(vX.Y.Z-<network>-<nonce>), but it doesn't need to follow a specific pattern.Breaking Changes
This is a breaking change and requires coordination for upgrades. The reason for that is nodes, as contracts, hash messages, and these hash functions must be the same. We could make a compatibility layer, but it'd require operators to track contract upgrades, so it's some added complexity.
It also requires contract upgrades!