Only allow auctions to be started at most once a month per token #254

KevinLiLu · 2018-06-30T01:47:29Z

Closes #185

Make auction start immediately upon creation
Only allow auctions to be started at most once a month (per token)

KevinLiLu · 2018-07-06T01:16:18Z

Hi @area, can I get a review? I'm also unable to change the label.

elenadimitrova · 2018-07-06T06:25:43Z

contracts/ColonyNetworkStorage.sol

@@ -70,4 +70,7 @@ contract ColonyNetworkStorage is DSAuth {
  uint256 reputationRootHashNNodes;
  // Mapping containing how much has been staked by each user
  mapping (address => uint) stakedBalances;
+
+  // Mapping containing the last auction start timestamp for a token address
+  mapping (address => uint) recentAuctions;


Just an idea to map token address to auction address and get the start time off that. This would give us a mapping for the latest token auction (which is not maintained on-chain currently but probably should be) and additionally avoid duplicating the startTime value.

Given that the auction contracts selfdestruct when complete, we'd have to check that there was still a contract there before trying to call startTime on it, otherwise we'd revert. That's fairly straightforward to do in assembly with the extcodesize call though.

ah yes, you're right. But we probably won't be able to determine if a month has gone by if the auction contract is destroyed, right?

Ah, also true, I hadn't considered that an auction could finish within the 30 days.

Let's leave it as is then. At most we might add the auction contract address record to this mapping at some point.

area · 2018-07-06T10:12:45Z

contracts/ColonyNetworkAuction.sol

@@ -28,10 +28,14 @@ contract ColonyNetworkAuction is ColonyNetworkStorage {
  event AuctionCreated(address auction, address token, uint256 quantity);

  function startTokenAuction(address _token) public {
+    uint lastAuctionTimestamp = recentAuctions[_token];
+    require(lastAuctionTimestamp == 0 || now - lastAuctionTimestamp >= 30 days, "An auction may only be started once every 30 days.");


We've been using kebab-case messages in requires. So something like colony-auction-start-too-soon would be appropriate here; the primary use case we're targeting is for clients to be able to interpret them with user-friendly messages, not for users to see them themselves.

area · 2018-07-06T10:19:57Z

contracts/ColonyNetworkStorage.sol

@@ -70,4 +70,7 @@ contract ColonyNetworkStorage is DSAuth {
  uint256 reputationRootHashNNodes;
  // Mapping containing how much has been staked by each user
  mapping (address => uint) stakedBalances;
+
+  // Mapping containing the last auction start timestamp for a token address
+  mapping (address => uint) recentAuctions;


Given that the auction contracts selfdestruct when complete, we'd have to check that there was still a contract there before trying to call startTime on it, otherwise we'd revert. That's fairly straightforward to do in assembly with the extcodesize call though.

KevinLiLu · 2018-07-10T05:22:06Z

@elenadimitrova @area

I was on vacation the last few days so I did not get around to working on this.

I just pushed an update to address the review comments. Can you mark this as ready-for-review again?

area

Looking good, only a couple of things left to address. When you've dealt with my comments, make sure to rebase on the currentdevelop branch too so we can merge it in.

area · 2018-07-10T10:39:55Z

contracts/ColonyNetworkAuction.sol

    address clny = IColony(metaColony).getToken();
    DutchAuction auction = new DutchAuction(clny, _token);
    uint availableTokens = ERC20Extended(_token).balanceOf(this);
    ERC20Extended(_token).transfer(auction, availableTokens);
+    auction.start();
+    recentAuctions[_token] = auction.startTime();


Why the call to auction.startTime() here? Why not just now?

I believe both evaluate to the same value. Does using auction.startTime() incur a higher cost?

Yeah, a call to an external contract is going to cost more than reading the state by about 2000 gas.

Got it. Latest push addresses all the comments and rebases onto current develop branch.

area · 2018-07-10T12:50:44Z

test/colony-network-auction.js

+      await token.mint(quantity.toString());
+      await token.transfer(colonyNetwork.address, quantity.toString());
+      await colonyNetwork.startTokenAuction(token.address);
+      const newAuctionStartTime = await tokenAuction.startTime.call();


Shouldn't be a need for the .call() here. I know we explicitly use call elsewhere in the file, but that's a relic from days gone by, and we've been trying to remove it where possible / not introduce more ones. Same on L194 as well.

Oh I see. I changed the old startTime.call() usage on L104 too.

area assigned KevinLiLu Jul 2, 2018

area added the in-progress label Jul 2, 2018

KevinLiLu force-pushed the feature/185-limit-auctions-to-once-a-month branch from 5f961d0 to 9a1011c Compare July 3, 2018 16:23

elenadimitrova added ready-for-review and removed in-progress labels Jul 6, 2018

elenadimitrova reviewed Jul 6, 2018

View reviewed changes

elenadimitrova requested a review from area July 6, 2018 06:25

area reviewed Jul 6, 2018

View reviewed changes

area added in-progress and removed ready-for-review labels Jul 6, 2018

KevinLiLu force-pushed the feature/185-limit-auctions-to-once-a-month branch from 9a1011c to d7a0fb9 Compare July 10, 2018 04:10

elenadimitrova added ready-for-review and removed in-progress labels Jul 10, 2018

area reviewed Jul 10, 2018

View reviewed changes

area added in-progress and removed ready-for-review labels Jul 10, 2018

KevinLiLu force-pushed the feature/185-limit-auctions-to-once-a-month branch 2 times, most recently from 356054d to 980c82e Compare July 11, 2018 04:50

Restrict auctions to once a month

bb8ffe4

KevinLiLu force-pushed the feature/185-limit-auctions-to-once-a-month branch from 980c82e to bb8ffe4 Compare July 11, 2018 07:22

elenadimitrova added ready-for-review and removed in-progress labels Jul 11, 2018

area approved these changes Jul 11, 2018

View reviewed changes

area merged commit 6674f0c into JoinColony:develop Jul 11, 2018

elenadimitrova removed the ready-for-review label Jul 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only allow auctions to be started at most once a month per token #254

Only allow auctions to be started at most once a month per token #254

KevinLiLu commented Jun 30, 2018 •

edited

Loading

KevinLiLu commented Jul 6, 2018

elenadimitrova Jul 6, 2018

area Jul 6, 2018

elenadimitrova Jul 6, 2018

area Jul 6, 2018

elenadimitrova Jul 6, 2018

area Jul 6, 2018

area Jul 6, 2018

KevinLiLu commented Jul 10, 2018

area left a comment

area Jul 10, 2018

KevinLiLu Jul 11, 2018 •

edited

Loading

area Jul 11, 2018

KevinLiLu Jul 11, 2018

area Jul 10, 2018

KevinLiLu Jul 11, 2018

Only allow auctions to be started at most once a month per token #254

Only allow auctions to be started at most once a month per token #254

Conversation

KevinLiLu commented Jun 30, 2018 • edited Loading

KevinLiLu commented Jul 6, 2018

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

KevinLiLu commented Jul 10, 2018

area left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinLiLu Jul 11, 2018 • edited Loading

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

KevinLiLu commented Jun 30, 2018 •

edited

Loading

KevinLiLu Jul 11, 2018 •

edited

Loading