-
Notifications
You must be signed in to change notification settings - Fork 0
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
Updates to ERC 7683 #1
base: master
Are you sure you want to change the base?
Conversation
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.
Great work!
ERCS/erc-7683.md
Outdated
|
||
All sub-types SHOULD be registered at github.com/erc-7683/filler-data-subtypes to encourage sharing of sub-types based on their functionality. | ||
|
||
### fillerData |
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.
What do you think about including a section on example user flow, or desired user flow? Something similar to the messages we sent Zain recently
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.
+1 I think that would be a good addition.
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 want to get some consensus around what the core structures look like, but once we have a good idea of what those should be, I think adding some user journeys would be great. If we are allowed to add visual diagrams, I think that could also be very useful.
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.
Yeah we are allowed to add diagrams. For example 4337 does 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.
Sick I really like all of the ideas here! A few comments that came to mind while reading through
/// @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 settlementContract; | ||
/// @dev The address of the user who is initiating the swap, | ||
/// whose input tokens will be taken and escrowed | ||
address swapper; | ||
address user; |
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 find user to be too vague here - there are many "users" in the system: fillers, swappers, potentially quoters, relayers etc. specificity seems useful here imo
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.
Yeah, I hear that. Any ideas?
Swapper is specific, but inaccurate (by colloquial reading), since this isn't always a swap.
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.
initiator ? 🤔
|
||
/// @title OnchainCrossChainOrder CrossChainOrder type | ||
/// @notice Standard order struct for user-initiated orders, where the user is the msg.sender. | ||
struct OnchainCrossChainOrder { |
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.
thoughts on composing gasless + onchain given the former is a superset of the latter?
ie
struct GaslessCrossChainOrder {
CrossChainOrder inner;
// ... this is the only contract that has permission to execute this order
address settlementContract;
// replay-protection nonce for the user's signature
uint256 nonce;
// maybe consider including signature in here to be even more clear?
}
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.
at least make the param ordering the same so it's clear what the diff is
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.
composition would also potentially allow code / interface sharing
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.
at least make the param ordering the same so it's clear what the diff is
Good call. Meant to do this. Probably just a mistake.
thoughts on composing gasless + onchain given the former is a superset of the latter?
I originally wanted to do it this way. Problem was that it makes it more confusing to read and annoying to construct since there's an extra layer of nesting. Not obvious to me that nesting makes things easier for the people building and touching this structure offchain.
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.
Corrected the ordering.
ERCS/erc-7683.md
Outdated
Cross-chain execution systems implementing this standard SHOULD create a custom sub-type that can be parsed from the arbitrary `orderData` field. This may include information such as the tokens involved in the swap, the destination chain IDs, fulfillment constraints or settlement oracles. | ||
Cross-chain execution systems implementing this standard SHOULD use a sub-type that can be parsed from the arbitrary `orderData` field. This may include information such as the tokens involved in the transfer, the destination chain IDs, fulfillment constraints or settlement oracles. | ||
|
||
All sub-types SHOULD be registered at github.com/erc-7683/order-subtypes to encourage sharing of sub-types based on their functionality. See the examples section for an example of how sub-types can be used to support behavior like executing calldata on a target contract of the user's choice on the destination chain. |
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: make the github repo a link
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.
Yeah, it's just a placeholder. Repo hasn't been created. Will create before proposing.
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.
Done.
ERCS/erc-7683.md
Outdated
/// @dev The timestamp by which the order must be initiated | ||
uint32 initiateDeadline; | ||
/// @dev The chainId of the origin chain | ||
uint32 originChainId; |
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 shouldn't be needed for onchain, right? since it's implicit by eip-155 in the user tx
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.
Yeah, probably not.
I do think it's quite common for desyncs to happen between a dapp and the wallet where the dapp thinks it's sending to chain A, but the wallet sends to chain B. We've seen this a bunch.
Probably not a big enough issue to warrant including the param here, though.
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.
Removed.
@@ -98,79 +111,240 @@ struct Input { | |||
struct Output { | |||
/// @dev The address of the ERC20 token on the destination chain | |||
/// @dev address(0) used as a sentinel for the native token | |||
address token; | |||
bytes32 token; |
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.
why 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.
assuming for non-EVM compatibility but then does uint32 chainId still apply?
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.
Yeah, non EVM compatibility.
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 question on chainId. First of all, does chainId have a well-defined size? I can't find any reference to its size, and I've seen uint256 used to represent it in many cases.
If it is limited to uint32, I would suggest updating it to some larger size (uint64 would be simplest) and reserving the top half of the bit space for non EVM.
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.
Updated all to uint64, but would be curious to get your thoughts on how chains without an EVM chainid should be handled.
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.
At Skip we are working on a system implementing 7683 and Cosmos chains use strings for chain IDs so this is something we're particularly unsure of how to handle.
Many messaging platforms just assign arbitrary numbers for these chains but I'm worried about running into an ID conflict down the road.
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.
Definitely something I'm worried about too.
In EVM world this is obvious, in non EVM world, this is less obvious. My first instinct is to treat the uint as a packed struct, where the first n
bytes are used as an ecosystem identifier and the last m
bytes are used as the chainId for that particular ecosystem. It makes it simpler to let ecosystems use whatever mapping they want as long as they don't need all 32 bytes (assuming we upsize this to uint256) for their ids.
Thoughts? Is there a standard way that people translate these strings into some integer, like a truncated hash, for instance, to get compatibility with EVM-style chainIds?
/// @dev The contract address that the order is meant to be settled by | ||
uint32 destinationChainId; | ||
/// @dev The contract address that the order is meant to be filled on | ||
bytes32 destinationSettler; |
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 this spec needs a glossary to explain what the different terminology means. not sure settler
is especially clear without that
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.
Agreed.
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.
Done.
/// @dev The contract address that the order is meant to be filled on | ||
bytes32 destinationSettler; | ||
/// @dev The data generated on the origin chain necessary to inform the fill | ||
bytes originData; |
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.
why originData
name? because it comes from the origin chain? Generally think name should be about what it does and where it goes rather than where it comes from. Maybe settlementData
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.
It was intended to differentiate from fillerData on the destination (since that is also used for settlement). Could also be called settlerData?
No strong opinions. Naming is hard
ERCS/erc-7683.md
Outdated
|
||
/// @notice Signals that an order has been initiated | ||
/// @param orderId a unique order identifier within this settlement system | ||
/// @param fillInstructions Fill instructions to parameterize each leg of the fill |
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.
explain why array + what leg means 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.
Agreed, can add more explanation.
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.
Done.
- **Destination Chain**: the chain where the intent is executed and the user recieves funds. Note: intents can involve multiple destination chains. | ||
- **Filler**: a participant who fulfils a user intent on the destination chain(s) and recieves payment as a reward. | ||
- **Leg**: a portion of the user intent that can be executed independently from others. All legs must be executed for an intent to be considered fulfilled. | ||
- **Origin chain**: the chain where the user sends funds. |
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.
would this system allow for multiple origin chains?
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, it wouldn't. The current standard assumes that there's only a single origin chain per order. These can be composed just by combining multiple 7683 orders on multiple origins into a larger construct.
Thoughts? Would this cause problems for your use case?
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.
The main problem I see is that it is unclear in this case which one of these independent orders should actually execute the calls on the target chain - especially in cases where it isn't clear which order would be able to be settled first. There might be ways of solving this problem outside of the protocol, but will have a think on 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.
@mrice32 Not allowing multiple source chains in the same Order struct will become problematic for our use case, and most probably many other smart account systems too.
A nice feature of smart accounts is they can allow selective replay of the same signature across different chains if the user wants.
So a user who is pulling funds from many different source chains and depositing to a single target chain would only need to sign one order, which would contain the chainIds of all of the source chains, and the smart account would just parse the order and check if the current chain ID matches any of the IDs signed in the order.
But to do this, the hash of the order should be the same for all chains. This can only be achieved if the order follows a pattern like -
ChainOrder {
uint256 originChainId
uint256 targetChainId
address originSettler
address targetContract
... }
Order {
ChainOrder[] orders
}
In this simplified example, the hash of the order could be the same for all origin chains, so the end-user only h as to make 1 signature for N chains.
There are some tricks which would let EOAs utilize this UX improvement too.
It seems like 7683 could support this pretty easily, by just packing all "per chain" information into a struct and then creating an array of these structs for the gaslessOrder and resolvedOrder.
I don't think this would add much gas or complexity to the current implementation.
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.
Okay, this is very interesting. A few questions:
- Why can't you just have a list of transactions that each contain an ERC-7683 open function calldata (order) that you want to send? And wouldn't this be more general anyway? Something like:
struct Transaction {
address to;
value: uint256;
callData: bytes;
chainId: uint256;
}
struct MultichainCall {
Transaction[] transactions;
// Per chain replay protection.
nonce: uint256;
}
// Call this on your smart account:
function multichainCall(MultichainCall memory multichainCall, bytes memory signature) {
// Check if nonce is used on this chain.
// Walk through all txns and execute any that have chainId == block.chainId.
}
- Is atomicity the issue? Note: the only functional difference I see in 7683 allowing multi-origin within the standard vs having users of the standard compose multiple orders to create a multi-origin order is per-destination atomicity. Out of the box, I can't guarantee that either all the orders go through or none of them do. But you may be able to do this through creating conditional relationships between the fills of different orders, but that can get complicated.
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 kind of what we have right now, and for just signature minimization this works fine. But we don't see a clear way in which we would be able to leverage 7683 interoperability with this construction.
I think the question is- What is the actual intent we want to be interopable?
There are 2 types of intents a user can sign -
A. "I want to bridge 100 USDC to Arbitrum and deposit them into Uniswap"
B. "I want to bridge 100 USDC from Base to Arbitrum and deposit them into Uniswap"
The current 7683 implementation without multiple origin chains supports only intents of type B, to support intents of type A they require you to break it down into an array of type B intents.
But these generated type B intents cannot be interoperable across different settlement systems individually because they require destination atomicity and destination ordering.
-
Destination Atomicity - Like you mentioned in point 2, this is a feature that can only be enabled when the settlement systems are aware of the multi-origin order. The settlement system has multiple ways of guaranteeing atomicity - like the conditional relationship that you mentioned, where you could enforce in the settlement contract that all the orders in a multi-origin order are filled together.
Another way to do this is to add this as a condition to the settlement system's oracle. So the oracle would simply check if the state transition happened atomically on the target chain like the user requested. -
Ordering - This is as important as atomicity, because in a settlement system ( like Across ) that allows arbitrary calls to be made on the destination, it is important that the actual call to deposit funds to Uniswap, is only made after all the funds have been bridged to the destination chain.
So if there are N origin orders - then 1 to N-1 orders just do bridging, and the Nth order does bridging + the deposit to Uniswap call.
Taking this Uniswap example further, let's say that we break down this type A order into the following 7683 type B orders -
B1. I want to bridge 50 USDC from Optimism to Arbitrum
B2. I want to bridge 50 USDC from Base to Arbitrum and deposit to Uniswap.
B1 and B2 are individually "unfillable", making them interoperable across a bunch of settlement layers is not useful because a solver with just the information about B1, will not be able to fill this order without also getting information about B2.
My understanding is that we can change the scope of 7683 from type B intents to type A intents pretty easily by just allowing multiple origin chains. Do you see a downside of doing this?
## Rationale | ||
|
||
### Generic OrderData | ||
|
||
A key consideration is to ensure that a broad range of cross-chain intent designs can work within the same standard. To enable this, the specification is designed around a standard cross-chain intents *flow*, while allowing for varying implementation details within that flow. | ||
A key consideration is to ensure that a broad range of cross-chain intent designs can work within the same standard. To enable this, the specification is designed around a cross-chain intents _flow_, with two variations: gasless and onchain. |
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.
imo gasless and onchain might not be the best terms since you can have a gasless tx (eg using paymasters) that happens in the "onchain flow" and technically the "gasless flow" is no less onchain than the other. the only salient difference between these two flows is which order they are in so terminology that refers to the order might be easier to understand
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 have any alternative suggestions? I don't love the term onchain either.
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 terms that mention the chronology make sense, here are some random ideas coming to mind (feel free to discard all of them but could be useful for brainstorming):
- pre-settle vs post-settle
- stepwise vs reverse
- stepwise vs instant
- pre-fund vs post-fund
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.
Would any of these be drop-in replacements for the two existing structs?
I'm having a little trouble mapping these chronology terms on the existing structs. Or are you suggesting functional changes?
ERCS/erc-7683.md
Outdated
/// @param signature The user's signature over the order | ||
/// @param fillerData Any filler-defined data required by the settler | ||
/// @returns fillInstructions instructions to parameterize each fill leg, with each element representing a single leg | ||
function initiateFor(GaslessCrossChainOrder order, bytes signature, bytes originFillerData) external returns (FillInstructions[]); |
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'm not fully sure I understand this function/naming. From the section below, I understood the gasless flow to not require any tx on the origin chain until after the intent is filled on the target chains. Is this not the case?
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.
It's not necessarily the case. The standard was originally built with the following gasless ordering in mind:
- Filler gets user signed order.
- Filler sends a transaction on the origin chain to escrow the user's tokens.
- Filler sends a fill on the destination(s).
2 generally comes before 3 in the classic flow to protect the filler from the user moving their tokens after the fill but before the filler escrows the user's tokens.
Other flows where the user pre-locks their tokens wouldn't need this ordering. The standard fully supports that, but the "classic" ordering is the reason for the naming. Do you have an alternative name for initiate that would better communicate this flexibility?
ERCS/erc-7683.md
Outdated
} | ||
``` | ||
|
||
In this example, the Message sub-type allows the user to delegate destination chain contract execution to fillers. However, because transactions are executed via filler, the `msg.sender` would be equal to the filler's account, making this `Message` sub-type limited if the target contract authenticates based on the `msg.sender`. Ideally, 7683 orders containing Messages can be combined with smart contract wallets like implementations of [ERC4337](https://github.com/across-protocol/ERCs/blob/master/ERCS/erc-4337.md) or [EIP7702](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-7702.md) to allow complete cross-chain delegated execution. |
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.
wouldnt the sender here be the IDestinationSettler
rather than the filler's account?
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're absolutely right. This is a mistake.
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.
Fixed.
83bdec5
to
9fdaf34
Compare
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.
Left a few small notes; I also think we should make it more clear that initiate
might not be required at all in certain cases
ERCS/erc-7683.md
Outdated
Output[] maxOutputs; | ||
/// @dev The outputs to be given to the filler as part of order settlement | ||
Output[] minFillerOutputs; | ||
Output[] maxSent; |
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'd name this maxSpent
rather than maxSent
as sent is more ambiguous whereas spent makes it clear that the filler is spending / providing the tokens
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.
Fixed.
ERCS/erc-7683.md
Outdated
/// @dev The minimum outputs that must to be given to the filler as part of order settlement. Similar to maxSent, it's possible | ||
/// that special order types may not be able to guarantee the exact amount at initiate time, so this should be considered | ||
/// a floor on filler receipts. | ||
Output[] minRecieved; |
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 looks like a typo; should be minReceived
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.
Fixed.
ERCS/erc-7683.md
Outdated
/// @param fillInstructions fillInstructions instructions to parameterize each fill leg, with each element representing a single leg | ||
event Initiated(bytes32 indexed orderId, FillInstructions[] fillInstructions); | ||
/// @param fillInstructions instructions to parameterize each fill leg, with each element representing a single leg | ||
event Initiate(bytes32 indexed orderId, ResolvedCrossChainOrder resolvedOrder); |
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.
just flagging that this will be a pretty beefy event; I'd guess that having this whole struct will add easily 5k to 10k gas
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.
Yep, agreed. Do you see a way to either reduce the size or get rid of the event without hurting filler UX?
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'd still contend that if the filler is the one calling this function, they already know what is going to be resolved here; I think we should either make the event optional or only emit the truly dynamic components of the resolved order
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.
Also, if a filler does need the event, they could trigger the initiation from a contract that then takes the returndata and uses it to encode and emit the event
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'd still contend that if the filler is the one calling this function, they already know what is going to be resolved here; I think we should either make the event optional or only emit the truly dynamic components of the resolved order
They aren't necessarily the one calling the function. That's only in the gasless flow where the user isn't msg.sender. Also, onchain state can influence the result of this function. For instance, a dutch auction might depend on the exact block timestamp.
Also, if a filler does need the event, they could trigger the initiation from a contract that then takes the returndata and uses it to encode and emit the event
In the gasless flow, I agree. But that's a pretty bad dev-x to require the filler to build a contract to understand what they need to do to fill.
One potential compromise is that the standard requires that either you emit the event or resolve
's return value cannot change intra-block (meaning eth_call
ing resolve
with that block number is guaranteed to give you the correct fill instructions. This feels pretty complex, though.
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.
in the non-gasless flow, I agree that either the event needs be emitted or that the resolve
return value needs to be consistent!
|
||
Standard cross-chain intents flow: | ||
#### Gasless cross-chain intents flow |
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.
@0age This section is pretty clear that the origin chain and destination chain actions don't have a pre-specified ordering. Where else do you think this should be noted?
Or do you think we should change the name of initiate? If so, do you have alternative suggestions?
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.
It could be named something like reserve
or lock
? No implicit suggestion of ordering that way, more scoped to the functionality
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.
lock
implies the locking of tokens, which might be presumptuous if this is called simultaneously with settlement. It's also overloaded with re-entrancy guards.
reserve
seems reasonable, albeit a little vague.
You good with reserve
?
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.
yeah, that seems good!
ERCS/erc-7683.md
Outdated
} | ||
|
||
/// @notice Tokens sent by the swapper as inputs to the order | ||
/// @notice Tokens sent by the user as inputs to the order | ||
struct Input { |
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 don't actually see this struct being used anywhere. Is there a typo somewhere, or am I misunderstanding what this is used for?
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.
Great point, will remove.
@@ -98,79 +111,240 @@ struct Input { | |||
struct Output { | |||
/// @dev The address of the ERC20 token on the destination chain | |||
/// @dev address(0) used as a sentinel for the native token | |||
address token; | |||
bytes32 token; |
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.
At Skip we are working on a system implementing 7683 and Cosmos chains use strings for chain IDs so this is something we're particularly unsure of how to handle.
Many messaging platforms just assign arbitrary numbers for these chains but I'm worried about running into an ID conflict down the road.
Are there plans to specify a standard interface for handling the repayment of the solver after an intent is filled? |
ERCS/erc-7683.md
Outdated
/// @param signature The user's signature over the order | ||
/// @param fillerData Any filler-defined data required by the settler | ||
/// @returns fillInstructions instructions to parameterize each fill leg, with each element representing a single leg | ||
function openFor(GaslessCrossChainOrder order, bytes signature, bytes originFillerData) external; |
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.
How do fillers know how to populate 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.
It's a great question. Since, fundamentally, it's meant to be arbitrary protocol-specific information populated by the filler, there is no standard way to tell fillers how to populate it.
All we can do is provide a place for the struct types to be shared: https://github.com/ERC-7683/subtypes/tree/main/filler-data-subtypes.
Thoughts?
|
||
/// @title OnchainCrossChainOrder CrossChainOrder type | ||
/// @notice Standard order struct for user-opened orders, where the user is the msg.sender. | ||
struct OnchainCrossChainOrder { |
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.
What's the reasoning behind standardizing this OnchainCrossChainOrder?
Backend entities ( fillers ) will just see the Open event anyway.
And independently this onchain order does not really have any details, the only thing that it enforces is the fillDeadline, the orderData is arbitrary.
How about letting origin chain contracts choose whatever form they want to receive the order in, and only standardize the Open event they broadcast for offchain entities to read.
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 question. You're right that it isn't much more than just some custom data. It could be bytes, but fillDeadline did seem like a field that's worth being shared. We have gotten feedback that a type field would also be useful here as well. Not super strongly opinionated, but if there are shared fields, it seems useful for the struct to be specified to be consistent with the Gasless one.
I do feel strongly that there should be an onchain open function, since the standard should support the (common) use case of the users being the msg.sender.
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 the question is how does standardizing this on-chain struct or having a standard open function help us with our goal to make an intent interoperable across different settlement systems?
Because if the intent is generated onchain, then the user already has to select the contract they want to call, and the orderData required.
Since this onchain struct or function call is not visible to any backend systems- they only see the Open event.
So do solvers/settlement systems care about if this "Open event" was generated through this OnchainOrder struct, or some other custom parameters of a function?
For example in Across, if the parameters of the deposit function changed, but the Deposit event emitted remained the same, then the whole system would still function correctly right?
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.
The idea is that the standard is intended to serve two groups: fillers and order creators. If I create an order and want to put it onchain, I want the function I call to be agnostic to the settlement system so my code stays the same no matter which settlement system I use for each order.
/// @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 settlementContract; | ||
address 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.
Should these addresses also be bytes32 to support non-EVM chains properly? Noticed this in a couple of other places too like the ResolvedCrossChainOrder
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 don't think so, but let me know if you agree with this logic.
- 7683 is intended to be cross compatible with other ecosystems via their own standards.
- Non EVM ecosystems will use their own structs and data formats, so those will not be shared.
- This field will only be emitted/used on chains that are EVM, since any non-EVM chain would have it's own cross-compatible 7683-like standard.
- Since this field refers to an address on the same chain as the contract implementing origin settler interface of 7683, we can assume 20 byte addresses are all that's needed.
Summary: we only use bytes32 addresses in cases where the account we're referring to can be on an arbitrary foreign chain, not a chain that we know is hosting contracts that implement 7683. Example of this would be an output token address or output recipient.
Thoughts?
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.
Yeah agreed, this makes sense.
struct CrossChainOrder { | ||
/// @title GaslessCrossChainOrder CrossChainOrder type | ||
/// @notice Standard order struct to be signed by users, disseminated to fillers, and submitted to origin settler contracts | ||
struct GaslessCrossChainOrder { |
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.
As mentioned in the open question to the PR, I think it could be really helpful if we add a bytes32 type field here. The reason for this is -
- It would allow offchain entities to go through through a large volume of 7683 orders by simply filtering the order types they support through this type field. In a world where there are millions of intents being generated every minute, decoding the order data of each of these to understand whether you want to fill this order or not becomes unreasonable.
- It would allow fillers to optimize resolving orders, by recreating the resolution logic of supported settlement systems offchain. Saving them an API call, and increasing speed for the end user.
- This would enable the creation of an onchain registry of orderData types, which we could standardize in the future as a replacement to the subtypes repo.
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.
Yeah, I agree with you. This is feedback we've gotten pretty frequently. The biggest downside is just bloating the order struct.
We will also need to standardize how these are generated (at least) and potentially have a registry to ensure uniqueness. My first thought is just to use a similar mechanism to EVM function and event identifiers, which are just hashes of a signature. And then we just don't worry about collisions.
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 added an orderDataType field to the standard. Right now this is just an EIP 712 typehash. However, I would ideally like this to also encapsulate the expected type for the filler data, since I think that would make things easier on fillers.
Any ideas?
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.
Yeah, 712 typehash is a good idea. It might be that a particular intent could be filled with multiple filler data types?
If the user wants to commit to a particular filler data type, then they could also add this to their order data struct?
/// @dev The address of the user who is initiating the swap, | ||
/// whose input tokens will be taken and escrowed | ||
address swapper; | ||
address user; | ||
/// @dev Nonce to be used as replay protection for the order | ||
uint256 nonce; |
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.
If the Gasless Order does not contain an explicit orderId, then solvers won't be able to pre fill the order on the destination chain before calling open on the source chain because they won't be able to deterministically compute the orderId.
Is there a reason why this nonce should differ from the orderId emitted in the Open event. Maybe we could reuse this as the orderId, instead of creating a new field for it.
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.
Edit: originally misread your question. Thought you were implying you couldn't use orderId.
I don't see why you couldn't do this.
If I were designing a pre-fill system, I might not use the naked nonce as the orderId (depending on the way the offchain order system is designed). Instead, I would probably hash the nonce with other fields that can't be spoofed, like user and/or msg.sender, so I can be sure the nonce can't be stolen by a malicious 3rd party that frontruns me.
Essentially, I think to do pre-fills you just want the orderId to be deterministically computable ahead of time and guaranteed to not be invalidated by some 3rd party initiating a state change before you.
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.
Yeah makes sense. But this requires the fillers to be aware of the logic that the settlement system will use to generate the orderId, and if they accidentally get this logic wrong, then they lose money, because they fill for an orderId that can't exist.
But I understand why it wouldn't make sense to add an orderId field explicitly into the struct , because it would take away the ability for settlement systems to maintain an ordered orderId on chain.
I think for our case we can just an orderId field to our orderData, which is constructed in a determinstic way like you mentioned.
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.
But this requires the fillers to be aware of the logic that the settlement system will use to generate the orderId, and if they accidentally get this logic wrong, then they lose money, because they fill for an orderId that can't exist.
This is a good point. I think this was an oversight. Predicting the orderId
should be part of the standard.
My intention was that fillers could use resolve to predict their orderId, but now I realize that they can't do that because orderId
isn't included in the struct returned by resolve. The two alternatives I see are:
- New method where you pass in an order and get an
orderId
. Problem: fillers need to call two methods if trying to determine their fill arguments. - Add
orderId
toResolvedCrossChainOrder
. Problem: it creates redundancy in theOpen
event (orderId
is emitted in the event in two places), which is inefficient.
Thoughts?
- **Destination Chain**: the chain where the intent is executed and the user recieves funds. Note: intents can involve multiple destination chains. | ||
- **Filler**: a participant who fulfils a user intent on the destination chain(s) and recieves payment as a reward. | ||
- **Leg**: a portion of the user intent that can be executed independently from others. All legs must be executed for an intent to be considered fulfilled. | ||
- **Origin chain**: the chain where the user sends funds. |
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.
@mrice32 Not allowing multiple source chains in the same Order struct will become problematic for our use case, and most probably many other smart account systems too.
A nice feature of smart accounts is they can allow selective replay of the same signature across different chains if the user wants.
So a user who is pulling funds from many different source chains and depositing to a single target chain would only need to sign one order, which would contain the chainIds of all of the source chains, and the smart account would just parse the order and check if the current chain ID matches any of the IDs signed in the order.
But to do this, the hash of the order should be the same for all chains. This can only be achieved if the order follows a pattern like -
ChainOrder {
uint256 originChainId
uint256 targetChainId
address originSettler
address targetContract
... }
Order {
ChainOrder[] orders
}
In this simplified example, the hash of the order could be the same for all origin chains, so the end-user only h as to make 1 signature for N chains.
There are some tricks which would let EOAs utilize this UX improvement too.
It seems like 7683 could support this pretty easily, by just packing all "per chain" information into a struct and then creating an array of these structs for the gaslessOrder and resolvedOrder.
I don't think this would add much gas or complexity to the current implementation.
No, there aren't right now. Our thought was that any standardization of that part of the flow would exclude some reasonable repayment system. If you have ideas, would love to hear them! |
} | ||
|
||
/// @notice Tokens that must be receive for a valid order fulfillment | ||
struct Output { | ||
/// @dev The address of the ERC20 token on the destination chain | ||
/// @dev address(0) used as a sentinel for the native token | ||
address token; | ||
bytes32 token; |
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 just discovered bytes32 does not work for Cosmos compatibility. For example IBC addresses are 68 bytes. Technically there is no upper bound here
ERCS/erc-7683.md
Outdated
/// @dev The destination chain for this output | ||
uint32 chainId; | ||
uint64 chainId; |
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.
Note we may want to change this to uint256 to support possible patterns like ERC7785, which would use a bytes32 identifier instead of integer chain ID's
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.
Done
The discussion here has gotten quite long (github UI seems to not be handling it particularly well). It has been super useful to me and a bunch of great changes have come of it. I feel like this is a good time to move this update over to the main ERCs repo. |
324e0b7
to
24ee81c
Compare
24ee81c
to
3cdc87a
Compare
3cdc87a
to
6dd691a
Compare
The commit 6dd691a (as a parent of 65edb52) contains errors. |
461aeb3
to
5cca59c
Compare
Signed-off-by: Matt Rice <[email protected]>
5cca59c
to
ac4656b
Compare
This PR proposes a few updates to ERC-7683. These updates are intended to make the language and functionality in the ERC capture more use cases than originally foreseen, require less (or no) protocol-specific code and casework for fillers, and provide visibility into the opaque bytes fields used in the standard.
Notable changes:
initiate
is now calledopen
.swap
has been replaced withtransfer
throughout.fill
function and process (i.e. the function on the destination chain).open
), the standard now requires that the resolved order includesFillInstructions
, which tells the filler what data needs to be included in which destination transactions. The resolved order is now returned byopen
and emitted in anOpen
event.orderData
subtypes andfillerData
subtypes.fill
happens beforeopen
.