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

[area]

Currently (or at least, once #168 is merged), auctions for a specific token can be started repeatedly and as often as people like. For a particular token, an auction should only be able to be started one month after the last auction was started. The intention for this is to ensure that a moderate number of tokens are sold off each time, and to reduce the burden on users who are interested in buying a particular token (they only have to track one auction per month, rather than an arbitrarily large number).

[KevinLiLu]

@area Can I take a stab at this?

My initial thought is to use a storage variable mapping (address => uint) recentAuctions; in ColonyNetworkAuction that maps the colony token address to the timestamp of the most recent auction for that colony token.

startTokenAuction would first do a require statement to verify either (1) token address not in mapping already, or (2) the difference between block.timestamp (?) and timestamp value in mapping is >= one month.

If the auction was successfully created, then we update the mapping with block.timestamp(?).

[area]

Of course! What you've outlined sounds correct. Just remember that the storage variable should be declared in ColonyStorage ColonyNetworkStorage, and to include (a) test(s).

[KevinLiLu]

@area Why should the mapping be stored in ColonyStorage? Did you mean the ColonyNetworkStorage contract as ColonyNetworkAuction inherits from ColonyNetworkStorage?

[area]

You're absolutely right. That's what I get for trying to be helpful before breakfast.

[KevinLiLu]

@area

Another thing: it looks like ColonyNetworkAuction.startTokenAuction only creates the DutchAuction contract, but an explicit call to DutchAuction.start() is required to actually start the auction. This means we will want to track the timestamp of when DutchAuction.start() is called right?

Seeing as a DutchAuction will not have direct access to the token address => timestamp mapping in ColonyNetworkStorage, i'm thinking that the mapping should instead map token address => the most recent DutchAuction contract itself. This way we can simply use DutchAuction.startTime when checking if 30 days have passed.

[area]

Another thing: it looks like ColonyNetworkAuction.startTokenAuction only creates the DutchAuction contract, but an explicit call to DutchAuction.start() is required to actually start the auction. This means we will want to track the timestamp of when DutchAuction.start() is called right?

It's clear why we're not doing the functionality in DutchAuction.start() in the constructor (it wouldn't have any tokens when the constructor was being run), but I don't see why we can't call .start() in startTokenAuction after the tokens have been transferred, which would make this easier for you and require jumping through the hoops you've described.

@elenadimitrova is more familiar with the decisions that went in to the auction code. She's on holiday right now, but tagging her so she can drop some knowledge when she finds her way back from whatever beach she's currently on.

[KevinLiLu]

I guess the question to answer is:

• Is there an advantage to allowing auctions to be in a CREATED state, but not yet STARTED/IN-PROGRESS.

If there is an advantage to a CREATED state, then we would probably want to rename the function to ColonyNetworkAuction.createTokenAuction to reflect the functionality.

Regardless, these are the approaches we have:

Approach 1 (requires calling .start() in startTokenAuction):
Use a storage mapping (address => uint) auctionCooldownExpirationTimes to store the timestamp when the next auction can be run for a token. If the token address does not exist in the mapping, then an auction can be run immediately.

We store the auction cooldown expiration time instead of the auction start time so we do not have to make calculations each time we want to check if an auction can be run (instead of checking now >= startTime + 30 days each time, we can simply check now >= auctionCooldownExpirationTime).

Approach 2 (if we cannot call .start() in startTokenAuction):
Use a storage mapping (address => DutchAuction) tokenAddrToAuctions to store each DutchAuction contract so we can easily access DutchAuction.startTime to determine if 30 days have passed.

Approach 3 (if we cannot call .start() in startTokenAuction):
Use a storage mapping (address => uint) auctionCooldownExpirationTimes to store the auction cooldown expiration time (or start time) and pass it into the DutchAuction constructor. The DutchAuction constructor will set the corresponding value.

[area]

We store the auction cooldown expiration time instead of the auction start time so we do not have to make calculations each time we want to check if an auction can be run (instead of checking now >= startTime + 30 days each time, we can simply check now >= auctionCooldownExpirationTime).

I don't see the benefit here? That calculation will be done on chain exactly once in either scenario, unless someone tries to start an auction without doing a .call() first to see if it'll be successful, in which case they're going to be wasting a lot more gas than they'd save from not doing the calculation 😄

Either way, I think we should go for Approach 1, moving the call to start() to the end of startTokenAuction. That's a touch more extensive that you originally signed up for, but is that still okay?

[KevinLiLu]

That would be the only benefit.

I am up for the challenge! 😄

[area]

Okay, well feel free to start working on it, and don't feel like you have to wait until you're done to open a PR; it's much easier for us to give feedback with a PR to look at, and lets us catch anything that appears to be going awry early.

[KevinLiLu]

@area

Got it. I've already created the PR that adds start() to end of startTokenAuction and modified the corresponding tests.

It also looks like I do not have permissions to set any fields in the PR (labels, assignee, etc).