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

[DRAFT] Encrypted Derivative Value (EDV) Auction Module #196

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Oighty
Copy link
Contributor

@Oighty Oighty commented May 20, 2024

See #194 .

This first pass shows that a large amount of code is duplicated from EMP. We can probably refactor the common parts into an intermediate (or 2) abstract contract. My rough thoughts for this are:

  • Auctions that store bids onchain in an array
  • Auctions that use the ECIES encryption/decryption library

@Oighty Oighty changed the base branch from master to fixed-batch May 20, 2024 21:55
@Oighty Oighty requested a review from 0xJem May 21, 2024 13:37
) internal override returns (uint64 bidId) {
// Decode auction data
(uint256 encryptedDerivativeValue, Point memory bidPubKey) =
abi.decode(auctionData_, (uint256, Point));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels better to define a struct for this an put it in the interface, so that integrators know what they're dealing with

///
/// @param price The fixed price of the auction (quote tokens per base tokens)
/// @param minValue The minimum derivative value that a user must bid to be considered in the auction
/// @param sortHighToLow True if bid values are sorted high to low, false if low to high
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reasoning for having this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be instances where you want the winners to be whoever bids the lowest value for some parameter. Some examples are: the duration of an option, the size of an additional incentive, the strike price of a put option.

Because of this minValue may need to conditionally be maxValue


// We cap the vesting duration to the max uint48 value
uint48 duration =
vestingDuration > type(uint48).max ? type(uint48).max : uint48(vestingDuration);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a check of whether start + duration > uint48 max

@0xJem
Copy link
Contributor

0xJem commented May 24, 2024

A few thoughts on abstraction of bid array and ECIES functionality:

  • There is definitely a lot of duplicated code for handling on-chain storage of bids
  • We should ideally design it so that the encryption method/algo can be easily changed. e.g. standardised _decryptBid() function that can have a different implementation to support a different encryption method.
  • Can a bid only have one decrypted value (e.g. amount out or derivative value), or can it be multiple values/a struct?
  • Another abstraction point is what to do with the decrypted value. e.g. contents of _decrypt(), performing a check on derivativeValue and inserting it into the queue.
  • Can the capacity and partial fill functionality be abstracted? Line 727 of EDV.sol

With respect to the timing... it's a pretty significant re-factor, and I feel that EMP would need to be re-audited. Do we want to hold up release for this?

@Oighty
Copy link
Contributor Author

Oighty commented May 28, 2024

A few thoughts on abstraction of bid array and ECIES functionality:

  • There is definitely a lot of duplicated code for handling on-chain storage of bids

Agree and think this should be it's own abstract.

  • We should ideally design it so that the encryption method/algo can be easily changed. e.g. standardised _decryptBid() function that can have a different implementation to support a different encryption method.

This might make sense. As is, it should be fairly easy to swap out to a different encryption library, but good to make it explicit.

  • Can a bid only have one decrypted value (e.g. amount out or derivative value), or can it be multiple values/a struct?

With the current library, it's limited to 32 bytes, but that could hold multiple values. We only use 16 bytes currently and use the others to provide a randomized mask, which makes it a little harder to brute force the decryption.

  • Another abstraction point is what to do with the decrypted value. e.g. contents of _decrypt(), performing a check on derivativeValue and inserting it into the queue.

Yeah, this seems easier that the decryptBid one.

  • Can the capacity and partial fill functionality be abstracted? Line 727 of EDV.sol

Maybe. It's the same in this case, but could be different if you use an algorithm that could potentially have multiple partial fills (e.g. VCG). I don't think this is as important at the moment though.

With respect to the timing... it's a pretty significant re-factor, and I feel that EMP would need to be re-audited. Do we want to hold up release for this?

I said all of the above to say. 1. I think we should create the abstractions for the EDV auction module and use them there, and 2. we shouldn't update the EMP module to use them right now. It makes sense to launch the EMP quickly and start getting some user feedback.

@0xJem
Copy link
Contributor

0xJem commented May 29, 2024

Will get to this once I've hit a wall on Baseline, or it's done.

@0xJem 0xJem changed the title Encrypted Derivative Value (EDV) Auction Module [DRAFT] Encrypted Derivative Value (EDV) Auction Module May 30, 2024
Base automatically changed from fixed-batch to develop June 6, 2024 13:26
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.

2 participants