-
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: Add relayer repayment address #653
Conversation
Signed-off-by: chrismaree <[email protected]>
Signed-off-by: chrismaree <[email protected]>
Signed-off-by: chrismaree <[email protected]>
Signed-off-by: chrismaree <[email protected]>
Signed-off-by: chrismaree <[email protected]>
This change looks clean to me; I don't have any comments to the changes. Will leave opportunity for others to comment before approving.
I think relayers will just deal with this but it's a headache for other integrators who are running infra against Across. Aggregators are an example - Socket and Li.Fi both have their own explorer implementations and they have specific knowledge about Across events. Wallet integrations is probably another stakeholder - Rabby can for instance decode Across transactions, so it'll probably impose some subtle breakage for them. It's unfortunate they we'll be breaking some integrations and will be imposing re-work, though I should note it's not the first time that this will be happening because we already have an ABI break for events in the current audit (RE deterministic deposit IDs). I don't think there's necessarily anything to do differently here - we should just be extra persuasive in encouraging integrators to migrate to our new app SDK, which will permit us to handle the breakage internally, such that others can make the transition by upgrading their package. |
contracts/SpokePool.sol
Outdated
@@ -1311,7 +1311,7 @@ abstract contract SpokePool is | |||
// exclusiveRelayer if passed exclusivityDeadline or if slow fill. | |||
function _fillRelayV3( |
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.
^
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.
Only one comment
contracts/SpokePool.sol
Outdated
@@ -1311,7 +1311,7 @@ abstract contract SpokePool is | |||
// exclusiveRelayer if passed exclusivityDeadline or if slow fill. | |||
function _fillRelayV3( | |||
V3RelayExecutionParams memory relayExecution, | |||
address relayer, | |||
bytes32 relayer, |
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 change this to repaymentAddress
for consistency?
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.
we could BUT it's called relayer
in the event and we don't really want to change the name there, do we?
I agree. I think we should batch any updates to interfaces/abis. |
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.
One open question is whether we should keep a backwards compatible fill interface.
This ties into how we're planning to orchestrate the migration. My straw man:
- We keep RelayData hash abi compatible (I think we can do this), so we know that old deposits and new deposits will collide if they have the same info.
- Both fill functions can still work with both the old deposit event and new deposit event.
- We keep both fill functions even after we do the deposit event upgrade, so relayers must start watching the new event, but can migrate the fill function they call at their own leisure. Old fill function is considered deprecated.
- We remove the old fill function in a later upgrade.
Thoughts?
I like this and it leans into why I wanted to keep the event structure the same and only change the relayer field :) |
@@ -256,7 +256,7 @@ pub fn execute_v3_slow_relay_leaf( | |||
fill_deadline: relay_data.fill_deadline, | |||
exclusivity_deadline: relay_data.exclusivity_deadline, | |||
exclusive_relayer: relay_data.exclusive_relayer, | |||
relayer: *ctx.accounts.signer.key, | |||
relayer: Pubkey::default(), // There is no repayment address for slow |
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: Why are we moving from the relayer public key to address_zero 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.
I think we had it wrong and EVM implementation passes empty address as well
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.
Looks good to me! Just had a question
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.
looking good
@@ -52,7 +52,7 @@ const DEFAULT_CONTRACT_COMPILER_SETTINGS = { | |||
optimizer: { enabled: true, runs: 1000000 }, | |||
viaIR: true, | |||
// Only strip revert strings if not testing or in ci. | |||
debug: { revertStrings: isTest ? "default" : "strip" }, | |||
debug: { revertStrings: isTest ? "debug" : "strip" }, |
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.
^
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.
Very odd 🤔
Signed-off-by: chrismaree <[email protected]>
This PR changes the interface of two existing EVM functions:
fillV3Relay
&fillV3RelayWithUpdatedDeposit
. They are extended to now containrepaymentAddress
as the address the relayer wants to get repaid on on chainrepaymentChainId
.one open question: do we want to support the old
fillV3Relay
interface, without this new prop, that just internally maps this address tomsg.sender
on EVM? this will enable current relayers to continue to work without requiring them to upgrade. main reason against having this backwards compatibility is we are adding breaking changes in other places (eg here: #650) that will also force relayes to upgrade so it could be argued that we should simple upgrade all the things at once without backwards compatibility in any location.