-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: address to bytes32 contract changes #650
Conversation
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
contracts/SpokePool.sol
Outdated
uint32 quoteTimestamp, | ||
uint32 fillDeadline, | ||
uint32 exclusivityPeriod, | ||
bytes calldata message | ||
) public payable override nonReentrant unpausedDeposits { | ||
// Check that deposit route is enabled for the input token. There are no checks required for the output token | ||
// which is pulled from the relayer at fill time and passed through this contract atomically to the recipient. | ||
if (!enabledDepositRoutes[inputToken][destinationChainId]) revert DisabledRoute(); | ||
if (!enabledDepositRoutes[inputToken.toAddress()][destinationChainId]) revert DisabledRoute(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we changed the data structure to:
mapping(bytes32 => mapping(uint256 => bool)) public enabledDepositRoutes;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think yes, as that would avoid unnecessary conversion, and it would still use the same storage slot for the mapped values.
Signed-off-by: Pablo Maldonado <[email protected]>
contracts/SpokePool.sol
Outdated
bytes calldata updatedMessage, | ||
bytes calldata depositorSignature | ||
) public override nonReentrant { | ||
_verifyUpdateV3DepositMessage( | ||
depositor, | ||
depositor.toAddress(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe also update _verifyUpdateV3DepositMessage interface so we don't need to translate to address here
pragma solidity ^0.8.0; | ||
|
||
library Bytes32ToAddress { | ||
function toAddress(bytes32 _bytes32) internal pure returns (address) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this also check that highest 12 bytes are all 0? Also, we should avoid using this method in comparisons where possible and convert the other value to bytes32 first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think that will get too expensive gas-cost wise? Maybe we can have safeToAddress and unsafeToAddress and use them depending on the context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra gas consumption associated with this check, this accounts for all the toAddress()
calls in each of this functions:
depositV3
: +128 gas units (from 100832
to 100960
)
fillV3Relay
: +156 gas units (from 109071
to 109227
)
I think it's negligable, wdyt?
contracts/SpokePool.sol
Outdated
@@ -979,7 +1009,7 @@ abstract contract SpokePool is | |||
) public override nonReentrant { | |||
V3RelayData memory relayData = slowFillLeaf.relayData; | |||
|
|||
_preExecuteLeafHook(relayData.outputToken); | |||
_preExecuteLeafHook(relayData.outputToken.toAddress()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we also change this _preExecuteLeafHook
contracts/SpokePool.sol
Outdated
@@ -141,9 +146,12 @@ abstract contract SpokePool is | |||
// token for the input token. By using this magic value, off-chain validators do not have to keep | |||
// this event in their lookback window when querying for expired deposts. | |||
uint32 public constant INFINITE_FILL_DEADLINE = type(uint32).max; | |||
/**************************************** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might not have meant to modify bits like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I have some issues with the formatter, I will try to fix my formatter or manually revert this changes
} | ||
|
||
library AddressToBytes32 { | ||
function toBytes32(address _address) internal pure returns (bytes32) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this. we should re-use it here as well:
function _addressToBytes32(address addr) internal pure returns (bytes32) { |
@@ -37,6 +38,8 @@ abstract contract SpokePool is | |||
{ | |||
using SafeERC20Upgradeable for IERC20Upgradeable; | |||
using AddressLibUpgradeable for address; | |||
using Bytes32ToAddress for bytes32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is actually pretty clean. good job.
Co-authored-by: Reinis Martinsons <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
…otocol/contracts into pablo/address-to-bytes32
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
@@ -225,8 +233,9 @@ abstract contract SpokePool is | |||
* @param payoutAdjustmentPct Adjustment to the payout amount. | |||
*/ | |||
/// @custom:audit FOLLOWING STRUCT TO BE DEPRECATED | |||
|
|||
struct RelayExecutionInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This data isn't actually used by the contracts and is only kept to give third-parties some way of scraping pre-v3 events. Updating the types here would defeat that purpose. But...it's been >6 months since any of these types/events were actually supported by the contracts. @nicholaspai @mrice32 Thoughts on just removing them?
Speculative PR here: #663
msg.sender.toBytes32(), | ||
recipient.toBytes32(), | ||
originToken.toBytes32(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good; deposit()
should ultimately be removed so there's no need for it to actually support bridging to SVM chains. Limiting it in this way is actually a good carrot to encourage people to get off deposit()
.
@@ -836,9 +864,9 @@ abstract contract SpokePool is | |||
// Exclusivity deadline is inclusive and is the latest timestamp that the exclusive relayer has sole right | |||
// to fill the relay. | |||
if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a PR here that factors this out to a function. Seems like that will go in, so it might be worth considering that here (it should permit a slight simplification of your change).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @pxrl. I can merge your changes once you merge them.
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
@@ -449,7 +450,7 @@ export async function getUpdatedV3DepositSignature( | |||
{ name: "depositId", type: "uint32" }, | |||
{ name: "originChainId", type: "uint256" }, | |||
{ name: "updatedOutputAmount", type: "uint256" }, | |||
{ name: "updatedRecipient", type: "address" }, | |||
{ name: "updatedRecipient", type: "bytes32" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
contracts/SpokePool.sol
Outdated
@@ -1039,16 +1237,17 @@ abstract contract SpokePool is | |||
SpokePoolInterface.RelayerRefundLeaf memory relayerRefundLeaf, | |||
bytes32[] memory proof | |||
) public payable virtual override nonReentrant { | |||
_preExecuteLeafHook(relayerRefundLeaf.l2TokenAddress); | |||
_preExecuteLeafHook(relayerRefundLeaf.l2TokenAddress.toBytes32()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we cannot change SpokePoolInterface.RelayerRefundLeaf
interface to use bytes32 for l2TokenAddress
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept it to prevent extra castings in the _distributeRelayerFunds function but you are right, I think these are negligible so I've updated the interface now.
@@ -15,6 +15,7 @@ import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; | |||
*/ | |||
contract Linea_SpokePool is SpokePool { | |||
using SafeERC20 for IERC20; | |||
using AddressToBytes32 for address; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: shall we use explicit imports? I see that recent commits have been moving in that direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @Reinis-FRP What do you mean here?
These are imported in the SpokePool.sol and used here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, meant using curly braces to explicitly state what contracts/libraries from source files we are importing
contracts/SpokePool.sol
Outdated
); | ||
} | ||
|
||
function _verifyUpdateV3DepositMessage( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could add comment why we need this internal overload and cannot convert to address at the outer function. I assume this has to do with different hash. Alternatively, we could change the interface of _verifyUpdateV3DepositMessage
to include hash type so that outer method could pass it through without needing overload on internal method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my answer here: https://github.com/across-protocol/contracts/pull/650/files#r1808446450
contracts/SpokePool.sol
Outdated
bytes memory updatedMessage, | ||
bytes memory depositorSignature | ||
) internal view { | ||
_verifyUpdateV3DepositMessageHelper( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need this extra helper? might be simpler to change the interface of _verifyUpdateV3DepositMessage to accept additional hashType parameter (see comment above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_verifyUpdateV3DepositMessage
is also overloaded with 2 implementations, one for address and one for bytes32
. I added the helper to reduce code duplication. Without it each _verifyUpdateV3DepositMessage
would need to implement _hashTypedDataV4(keccak256(abi.encode...
having this logic duplicated. Does it make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant avoiding overload in _verifyUpdateV3DepositMessage and let its caller in the external function to do address->bytes32 conversion when needed, as well as pass the relevant hashType from constants.
@@ -6,10 +6,10 @@ pragma solidity ^0.8.0; | |||
struct GaslessCrossChainOrder { | |||
/// @dev The contract address that the order is meant to be settled by. | |||
/// Fillers send this order to this contract address on the origin chain | |||
address originSettler; | |||
bytes32 originSettler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this require any changes in ERC proposal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. the ERC itself. is being updated by Matt and others.
uint32 exclusivityDeadline, | ||
bytes calldata message | ||
) external payable; | ||
|
||
function depositV3( | ||
address depositor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to support backwards overload in the interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I've removed them from the interface 👍
@@ -88,7 +81,7 @@ abstract contract CircleCCTPAdapter { | |||
* @param amount Amount of USDC to transfer. | |||
*/ | |||
function _transferUsdc(address to, uint256 amount) internal { | |||
_transferUsdc(_addressToBytes32(to), amount); | |||
_transferUsdc(to.toBytes32(), amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat that we can reuse the lib here :)
* If the depositor is a contract, it should implement EIP1271 to sign as a contract. See `_verifyUpdateV3DepositMessage()` | ||
* for more details on how the signature should be constructed. | ||
*/ | ||
function speedUpV3Deposit( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: is it useful to have both this address
and bytes
versions? do we expect intergrators (UIs) to call the address
or bytes
version? the context of my question is that seeing we wont support speedup on svm domain (requiring bytes32) is it useful to have this?
I think yes: we should have it, as we can then support it going forward and be consistent with the interface changes , but just through I'd bring it up to see what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so! But open to remove them if someone disagrees.
function _checkSwapOutputAndDeposit( | ||
uint256 swapTokenAmount, | ||
uint256 swapTokenBalanceBefore, | ||
uint256 inputTokenBalanceBefore, | ||
uint256 minExpectedInputTokenAmount, | ||
DepositData calldata depositData, | ||
IERC20 _swapToken, | ||
IERC20 _acrossInputToken | ||
bytes32 _swapToken, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is not your change, but its funny to me that these two variables are _
and the others are not.
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Changes proposed in this PR: