Skip to content
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

Use arbitrary data instead of quoteId #21

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fedgiac
Copy link
Contributor

@fedgiac fedgiac commented Sep 14, 2022

This is a naïve implementation of @nlordell suggestion of using more general arbitrary extra data instead of explicitly specifying quoteId, as in the future we might want to transmit different data to the backend from the contract (e.g. future new parameters or modified parameter types).

Unfortunately, it looks like these changes increase the gas cost of creating an order of about 700 gas:

╭────────────────────────────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮
│ Function Name                                  ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ createOrder (main)                             ┆ 0               ┆ 28870 ┆ 35066  ┆ 35066 ┆ 23      │
│ createOrder (this PR)                          ┆ 0               ┆ 29520 ┆ 35823  ┆ 35823 ┆ 23      │
│ (diff)                                         ┆ 0               ┆ +650  ┆ +757   ┆ +757  ┆ -       │
╰────────────────────────────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯

Considering that the ETH flow contract is easily replaceable in case we change the format of our backend, my preference goes against these changes, even if I consider using extraData cleaner and easier in terms of making the code understandable.

CC @josojo.

@nlordell
Copy link
Contributor

Change looks good! I think that the code here looks nicer and that this scheme is more future proof.

On the other hand, 750 gas per order is a non-negligible amount. This was a surprise to me (given that this should be a calldatacopy instruction). Since as @fedgiac the contracts are "disposable" in that we can deploy new ones, I also agree that it may not be worth it.

Some other things to consider when talking about re-deploying contracts like this:

  • There may be additional costs associated with auditing
  • We may have to shoe-horn changes in the services to make sure that we don't break this quote-ID format (things like encoding additional flags in the 32-bits of uint256 data - which is not unreasonable).

@josojo
Copy link
Contributor

josojo commented Sep 19, 2022

We have decided not to have this PR in the first version. But once we update the smart contract, in order to deal with Nic's excellent update order strategy, we might also add this PR again. Hence, let's keep it open for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants