Change switchboard and socket address types to bytes32 #135
Conversation
…that can touch multiple chains(non EVM also)
gwalen
force-pushed
the
dev-solana-update-types-bytes32
branch
from
2c372c8 to
3ef097d
Compare
| @@ -2,6 +2,7 @@ | |||
| pragma solidity ^0.8.21; | |||
|
|
|||
| import "./DeliveryUtils.sol"; | |||
| import { toBytes32Format } from "../../../utils/common/Converters.sol"; | |||
There was a problem hiding this comment.
toBytes32Format converter is imported in a bunch of contracts but not used.
| switchboard: watcherPrecompileConfig().switchboards(chainSlug_, sbType), | ||
| target: address(0), | ||
| switchboard: switchboardAddress, | ||
| target: toBytes32Format(address(0)), |
There was a problem hiding this comment.
Lets just do target: bytes32(0) instead of initializing address and then converting to bytes32.
|
|
||
| _plugConfig.appGatewayId = appGatewayId_; | ||
| // This is used by watcher precompile which need to also handle bytes32 format for Solana |
There was a problem hiding this comment.
Not sure I understand the comment, who is it intended for?
There was a problem hiding this comment.
This comment is no longer relevant - I’ll remove it
| struct AppGatewayConfig { | ||
| address plug; | ||
| bytes32 plug; | ||
| bytes32 appGatewayId; | ||
| address switchboard; | ||
| bytes32 switchboard; | ||
| uint32 chainSlug; | ||
| } |
There was a problem hiding this comment.
I think appGateway in AppGatewayConfig can be converted to address since it is only used locally on evmx. lets verify once.
There was a problem hiding this comment.
appGatewayId was bytes32 from the beginning, even before I started changing the types. It’s also referred to as appGatewayId, not just appGateway, which implies that it’s used as an identifier rather than an address. That could be the reason why it was defined as bytes32, so maybe it’s worth keeping it that way?
Maybe we could consider leaving this change for later. Updating these types takes quite a bit of time - both in Solidity and in the related TypeScript (deploy/connect) files. For the sake of the POC, we could stick to the minimal required change set and push redesign changes like the appGatewayId type update to a later stage (probably we should also rename it to appGateway and remove Id from it if changing type to address).
wdyt?
There was a problem hiding this comment.
yes it was already bytes32, but that is because we did not have the proper plan of what things have to be bytes32 and which ones are address. Since we have that now I was suggesting we make additional changes to keep it aligned.
But this doesnt strictly affect the POC outcomes so we can keep if you think the change will take lot of effort. We can pick it later.
| struct PlugConfigGeneric { | ||
| bytes32 appGatewayId; | ||
| bytes32 switchboard; | ||
| } |
There was a problem hiding this comment.
Similar to previous case, PlugConfigGeneric can also have address appgateway instead of bytes32
| function toBytes32Format(address addr) pure returns (bytes32) { | ||
| return bytes32(uint256(uint160(addr))); | ||
| } | ||
|
|
||
| function fromBytes32Format(bytes32 bytes32FormatAddress) pure returns (address) { | ||
| if (uint256(bytes32FormatAddress) >> 160 != 0) { | ||
| revert NotAnEvmAddress(bytes32FormatAddress); | ||
| } | ||
| return address(uint160(uint256(bytes32FormatAddress))); | ||
| } |
There was a problem hiding this comment.
I think we have redundant functions in WatcherIdUtils.sol
lets check and collapse.
There was a problem hiding this comment.
you are right, in WatcherIdUtils we have similar functions for en/decodeAppGatewayId I will remove them and instead use toBytes32Format/fromBytes32Format as more generic and used across EVMx and EVM, ok ?
There was a problem hiding this comment.
yes lets use toBytes32Format/fromBytes32Format everywhere
| bytes32 plug; | ||
| bytes32 appGatewayId; |
There was a problem hiding this comment.
same appGatewayId to address
| struct DigestParams { | ||
| address socket; | ||
| bytes32 socket; | ||
| address transmitter; | ||
| bytes32 payloadId; | ||
| uint256 deadline; | ||
| CallType callType; | ||
| uint256 gasLimit; | ||
| uint256 value; | ||
| bytes payload; | ||
| address target; | ||
| bytes32 target; |
There was a problem hiding this comment.
we have socket and target as bytes32 in digest, while digest is used on Socket contract in execute function, dont we need to convert these to address? not sure if things work in current form
There was a problem hiding this comment.
its fine to edit socket for this conversion from bytes32 to address since we decided everything has to be bytes32 in digest.
There was a problem hiding this comment.
to clarify socket and target should be bytes32 for digest to operate on bytes32for on-chain contract addresses (current PR code is ok) ?
--- side question: why there is a field address transmitter if transmitter is an off-chain app ?
There was a problem hiding this comment.
The structure of DigestParams is ok in current pr yes. It is the code on Socket that will need to be changed to work with this structure. Maybe we can leave it for now until you hit socket contract when testing with transmitter processor and watcher processor changes.
Change is applied all places that can touch multiple chains(non EVM also)
Cr fixes: