Big rewrite of hash register to be explicitly state-machine based #35
Conversation
e27c675
to
dd03cf6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some comments on some logic steps, but otherwise, brilliant additions
// Auction -> Reveal | ||
// Reveal -> Owned | ||
// Reveal -> Open (if nobody bid) | ||
// Owned -> Forbidden (invalidateName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't you be able to go from any state into forbidden?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing so complicates the bidding process a lot - so with this PR you can only forbid auctions that have finished. We could potentially add an Open -> Forbidden transition too.
// Too late! Bidder loses their bid. | ||
bid.closeDeed(0); | ||
BidRevealed(_hash, _owner, actualValue, 1); | ||
} else if(auctionState != Mode.Reveal) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you not be able to reveal it prematurely? You should not do it, but I'm not sure we should forbid it:
The issues with revealing too soon is that you someone might overbid you, or worse, add a second-place bid in purpose just so they can make you spend unnecessary funds (or maybe blackmail you into it)
But the issue with revealing too late is that you lose 100% of the funds and you lose the opportunity to buy the name. This is a much worse scenario.
In the other hand, by not forbidding, it might make users get in the habit of premature revealing it on the app because they're impatient, which is worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another consequence of simplifying the code to be explicitly state-based; allowing reveal in multiple states would complicate the code. I don't think there's a compelling reason to complicate the code to allow early reveals.
@@ -412,12 +434,11 @@ contract Registrar { | |||
* @param unhashedName An invalid name to search for in the registry. | |||
* | |||
*/ | |||
function invalidateName(string unhashedName) { | |||
function invalidateName(string unhashedName) inState(sha3(unhashedName), Mode.Owned) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, if a name could be invalidated before auctions take place, then bids would be returned.
bid.setBalance(actualValue); | ||
|
||
var auctionState = state(_hash); | ||
if(auctionState == Mode.Owned) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we allow auctions to be invalidated at any point, then we could add a if invalid
that would refund 100% of bids
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above - with this PR, at least, you can't invalidate during an auction.
@@ -427,6 +448,7 @@ contract Registrar { | |||
h.deed.closeDeed(1000); | |||
} | |||
HashInvalidated(hash, unhashedName, h.value, now); | |||
h.deed = Deed(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting solution!
@@ -217,14 +218,10 @@ describe('SimpleHashRegistrar', function() { | |||
function(done) { | |||
async.each(bidData, function(bid, done) { | |||
web3.eth.getBalance(bid.account, function(err, balance){ | |||
// Sleep sort is Best sort | |||
// setTimeout(function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ops totally forgot about this comment! :}
Deployment files for dnssec rollout
No description provided.