Auctions #6

Namaskar-1F64F · 2022-01-27T14:54:20Z

This PR adds a PorterAuction contract. This contract:
Inputs an EasyAuction address in the constructor.
Inputs an CollateralData into configureCollateral to deposit collateral and record that it made it onto the contract.
Inputs AuctionType.AuctionData, BondData, and CollateralData into createAuction which verifies requirements, updates collateral values, and calls initiateAuction.
Inputs AuctionType.AuctionData into initiateAuction which initiates a Gnosis Auction.

RusseII · 2022-01-27T15:06:23Z

Other than ad6f00b this is just adding the stuff from easy auction? Would probably make sense to merge this + most of the base easyAuction stuff then make modifications in follow up PRs

RusseII · 2022-01-27T15:15:57Z

Any idea what the common convention is when absorbing another repo like this? Would it make sense for us to start with a fork?

Namaskar-1F64F · 2022-01-27T18:25:57Z

Other than ad6f00b this is just adding the stuff from easy auction? Would probably make sense to merge this + most of the base easyAuction stuff then make modifications in follow up PRs

11bd375 are tests specifically for the custom Auction wrapper with some things copied & adapted from their testing code.

Namaskar-1F64F · 2022-01-27T18:27:45Z

Any idea what the common convention is when absorbing another repo like this? Would it make sense for us to start with a fork?

Starting with a fork could make sense. I think we'd want all of their tests against the EasyAuction as any modifications to that code should be reflected in the tests.

RusseII · 2022-01-27T18:44:46Z

Starting with a fork could make sense. I think we'd want all of their tests against the EasyAuction as any modifications to that code should be reflected in the tests.

The decision is up to you.

contracts/CollateralLockerFactory.sol

contracts/EasyAuction.sol

.vscode/extensions.json

Namaskar-1F64F · 2022-02-03T18:07:57Z

Un-drafting this. Updated description.

RusseII · 2022-02-03T18:45:29Z

Should we pull the gnosis auction stuff out of this repo? Have you had a chance to think about if we can make the entire e2e flow work without modifying gnosis auction?

Would you be able to add a loom video or similar walking through this? I'm not sure what I should be looking at exactly as there's ~2000 additions and 25 new files

Edit: I should only be looking at PorterAuction?

Namaskar-1F64F · 2022-02-03T18:55:29Z

Should we pull the gnosis auction stuff out of this repo? Have you had a chance to think about if we can make the entire e2e flow work without modifying gnosis auction?

I am not confident on the entire flow yet, but so far there isn't a reason to deploy gnosis auction. Let me test importing the auction from their github.

Would you be able to add a loom video or similar walking through this? I'm not sure what I should be looking at exactly as there's ~2000 additions and 25 new files

sure

Edit: I should only be looking at PorterAuction?

Good question. Appreciate you looking at this. Most of the files are GnosisAuction, yes. The files that are reviewable are:

PorterAuction.sol
AuctionE2E.spec.ts
utilities.ts (that are used in testing)

RusseII · 2022-02-03T19:08:29Z

I am not confident on the entire flow yet, but so far there isn't a reason to deploy gnosis auction. Let me test importing the auction from their github.

Deploy how? Can you do a mainnet fork for testing? Importing through github sounds good.

I created #14 where we can discuss wrapping gnosis auction in more depth

Good question. Appreciate you looking at this. Most of the files are GnosisAuction, yes. The files that are reviewable are:

Sounds good. Will take a look this afternoon.

luckyrobot

👏 so nice!

luckyrobot · 2022-02-03T22:51:17Z

contracts/PorterAuction.sol

+        address bondTokenAddress
+    );
+    event CollateralDeposited(
+        address indexed collateralDepositor,


can this be owner() ? or msg.sender ?

luckyrobot · 2022-02-03T22:51:20Z

contracts/PorterAuction.sol

+    /// @param creator the caller of the auction
+    /// @param auctionId the id of the auction
+    event AuctionCreated(
+        address indexed creator,


can this be owner() ? or msg.sender ?

I don't understand, could you clarify?

to remove the creator parameter, could we just use msg.sender?

contracts/PorterAuction.sol

luckyrobot · 2022-02-03T22:53:44Z

contracts/PorterAuction.sol

+        AuctionType.AuctionData memory auctionData,
+        BondData memory bondData,
+        CollateralData memory collateralData
+    ) public returns (uint256 auctionCounter) {


does this need an onlyOwner modifier or can anyone create one? same with the other functiosn ^

Good question. I think configureCollateral and createAuction are fine since they are supposed to be called by the user, but initiateAuction should probably be private.

luckyrobot · 2022-02-03T22:54:37Z

contracts/PorterAuction.sol

+        ] += collateralData.collateralValue;
+    }


should we emit() an event here when its all done?

The end of initiateAuction emits, unless you think there's value in emitting the collateral in contract updating. The flow does seem weird here. I'll think about this more.

luckyrobot · 2022-02-03T22:56:01Z

contracts/PorterAuction.sol

+        auctionId = gnosisAuction.initiateAuction(
+            IERC20(auctioningToken),
+            auctionData._biddingToken,
+            auctionData.orderCancellationEndDate,
+            auctionData.auctionEndDate,
+            auctionData._auctionedSellAmount,
+            auctionData._minBuyAmount,
+            auctionData.minimumBiddingAmountPerOrder,
+            auctionData.minFundingThreshold,
+            auctionData.isAtomicClosureAllowed,
+            auctionData.accessManagerContract,
+            auctionData.accessManagerContractData


does any of this data need to be validated before sending to gnosis? or do they do that?

They check all of the params, but I will wrap the approve in a require.

RusseII

🙏🙏 amazing 🙏🙏

contracts/PorterAuction.sol

RusseII · 2022-02-03T22:40:54Z

contracts/PorterAuction.sol

+/// @author Porter
+/// @notice This allows for the creation of an auction
+/// @dev This deviates from EasyAuction by not having an auctioning token until the auction is settling
+contract PorterAuction {


https://docs.porter.finance/product/spec/pages/create_auction_page#stages All the currently anticipated v1 configs are here - it makes sense to start with a simpler config tho

RusseII · 2022-02-03T22:41:28Z

contracts/PorterAuction.sol

+    struct BondData {
+        address bondContract;
+        uint256 maturityDate;
+        uint256 maturityValue;


I'm not sure what maturityValue is

RusseII · 2022-02-03T22:42:32Z

contracts/PorterAuction.sol

+
+    struct CollateralData {
+        address collateralAddress;
+        uint256 collateralValue;


Instead of value this should be # tokens I think. As far as I'm aware we don't have a way to tell the value without oracles in our smart contracts

Good point. I am not thinking in
t o k e n s
..yet

RusseII · 2022-02-03T22:46:36Z

contracts/PorterAuction.sol

+        console.log("Auction/createAuction");
+        // only create auction if there is no fee (will need to redeploy contract in this case)
+        require(
+            gnosisAuction.feeNumerator() == 0,


Does metamask give a warning if a user tries to make this transaction that it will fail? Or will the user submit the transaction, then it fails?

yes if I had to guess metamask does some rudimentary execution of the transaction, or at least parses some of the require statements. Here I am trying to renounce ownership of opensea. (It's really expensive)

contracts/PorterAuction.sol

RusseII · 2022-02-03T23:18:18Z

contracts/PorterAuction.sol

+                bondData.maturityDate >= auctionData.auctionEndDate,
+            "createAuction/invalid-maturity-date"
+        );
+        // Remove collateral from contract mapping before creating the auction


Interesting. I wonder if this pattern is best over keeping the collateral in the same place and locking it with an additional parameter etc

Yeah, I'm not sure yet.

RusseII · 2022-02-03T23:20:57Z

contracts/PorterAuction.sol

+            "configureCollateral/sender-inadequate-collateral"
+        );
+        require(
+            collateralToken.transferFrom(


Should this be using safeTransfer? https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol

I think so. I tried and it said the function didn't exist, since i'm not using SafeERC20, but I think the only downside is a little more gas usage.

RusseII · 2022-02-03T23:21:34Z

contracts/PorterAuction.sol

+    /// @dev The collateral is mapped from the msg.sender & address to the collateral value.
+    /// @dev Required msg.sender to have adequate balance, and the transfer to be successful (returns true).
+    /// @param collateralData is a struct containing the address of the collateral and the value of the collateral
+    function configureCollateral(CollateralData memory collateralData) public {


We will prompt them to approve the ERC20 token before they call this method on the frontend?

AFAIK, we can't approve for them, so something will have to prompt them beforehand.

contracts/PorterAuction.sol

RusseII · 2022-02-04T22:29:48Z

@Namaskar-1F64F do you need anything else from me or luckyrobot on this? I think it's good to get feedback from peteris or stu now.

Pet3ris

Great progress.

Some additional notes:

Worth adding unit testing by mocking Gnosis auction
Check https://github.com/Rari-Capital/solcurity for variables and functions to make sure you are using appropriate modifiers
Always think through access control (who is allowed to access each function) and state transitions (when is each function accessible), worth writing these down as a spec

contracts/Broker.sol

Separate gnosis auction contracts into their own folder

… auctions

RusseII

🙏

= added 3 commits January 27, 2022 09:53

Add EasyAuction and its dependencies

1e15fea

Add Auction and QaraghandyToken Contracts

ad6f00b

Add auction tests and its utilities

11bd375

remove token add collateral locker

048d71f

RusseII reviewed Jan 28, 2022

View reviewed changes

contracts/CollateralLockerFactory.sol Outdated Show resolved Hide resolved

contracts/EasyAuction.sol Outdated Show resolved Hide resolved

contracts/EasyAuction.sol Outdated Show resolved Hide resolved

add test config & vscode extension recommendation

ba82f5d

luckyrobot reviewed Feb 1, 2022

View reviewed changes

.vscode/extensions.json Show resolved Hide resolved

Namaskar-1F64F added 6 commits February 3, 2022 09:30

create fake erc20 tokens for testing

a899601

use default easy auction (revert changes)

a00ed91

(Auction => PorterAuction) add collateral/create

e5fa248

Redo Auction spec (currently not running & broken)

b190875

Add e2e spec

1b4ed7f

align with gnosis for multiple sell order support

873b5b4

Namaskar-1F64F marked this pull request as ready for review February 3, 2022 18:01

Remove unnecessary auction counter

3717ea4

luckyrobot reviewed Feb 3, 2022

View reviewed changes

RusseII reviewed Feb 3, 2022

View reviewed changes

code review feedback

2b74a80

renaming and such sorry!

b6226ab

Pet3ris reviewed Feb 7, 2022

View reviewed changes

Namaskar-1F64F and others added 16 commits February 7, 2022 08:35

update env example and change gas env key

77f6748

update license to agpl

97ed327

update function visibility

0059b42

Change ERC20 uses to IERC20

38e157c

remove unused porterbond

17d8606

simplify auction creation, rename auction->Broker

a556f4b

ownable and soon to be nonReentrant

2389698

move ido-contracts to own folder

a5cbdd9

Merge pull request #22 from porter-finance/separate-auction

45f8379

Separate gnosis auction contracts into their own folder

remove auction contracts

1b32e8b

use external auction

406b218

upgrade

7daf5b6

use pinned mainnet

545958d

reset fork on re-test

b1bdebe

format

e0c8fa2

Merge branch 'main' of https://github.com/porter-finance/v1-core into…

4c319fa

… auctions

RusseII approved these changes Feb 9, 2022

View reviewed changes

Namaskar-1F64F merged commit f6719a1 into main Feb 9, 2022

Namaskar-1F64F deleted the auctions branch February 9, 2022 00:42

RusseII mentioned this pull request Feb 9, 2022

Add auction contract + add auction to technical spec #3

Closed

Auctions #6

Auctions #6

Conversation

Namaskar-1F64F commented Jan 27, 2022 • edited Loading

RusseII commented Jan 27, 2022

RusseII commented Jan 27, 2022

Namaskar-1F64F commented Jan 27, 2022

Namaskar-1F64F commented Jan 27, 2022

RusseII commented Jan 27, 2022

Namaskar-1F64F commented Feb 3, 2022

RusseII commented Feb 3, 2022 • edited Loading

Namaskar-1F64F commented Feb 3, 2022

RusseII commented Feb 3, 2022

luckyrobot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RusseII left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RusseII commented Feb 4, 2022

Pet3ris left a comment

Choose a reason for hiding this comment

RusseII left a comment

Choose a reason for hiding this comment

Namaskar-1F64F commented Jan 27, 2022 •

edited

Loading

RusseII commented Feb 3, 2022 •

edited

Loading