Skip to content

[Protocol3] Improved maintenance mode#212

Merged
Brechtpd merged 10 commits intomasterfrom
maintenance-mode
Jun 10, 2019
Merged

[Protocol3] Improved maintenance mode#212
Brechtpd merged 10 commits intomasterfrom
maintenance-mode

Conversation

@Brechtpd
Copy link
Contributor

@Brechtpd Brechtpd commented Jun 8, 2019

Implements #206.

Tests still need to be updated.

@Brechtpd Brechtpd requested a review from dong77 June 8, 2019 19:49
uint numMinutesLeft = S.getNumDowntimeMinutesLeft();
if (numMinutesLeft < durationMinutes) {
uint numMinutesToPurchase = durationMinutes.sub(numMinutesLeft);
uint costLRC = getDowntimeCostLRC(S, numMinutesToPurchase);
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be written as:
uint costLRC = S.getDowntimeCostLRC(numMinutesToPurchase);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't seem possible because it's a function in the same library. If you do something like this:

library ExchangeAdmins
{
    using ExchangeAdmins for ExchangeData.State;
    ...
}

then this strangely compiles, but cannot be deployed because it references itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.

return S.numDowntimeMinutes;
} else {
// Calculate how long (in minutes) the exchange is in maintenance
uint numDowntimeMinutesUsed = now.sub(S.downtimeStart) / 60;
Copy link
Contributor

Choose a reason for hiding this comment

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

use block.timestamp instead of now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any reason for this? now is exactly the same as block.timestamp:
https://solidity.readthedocs.io/en/develop/units-and-global-variables.html?highlight=block.timestamp#block-and-transaction-properties

All other code uses now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, there is a lint warning we should use block.timestamp instead of now. But if you don't see it, it means we disabled that rule. Then stick to now for consistence then.

@dong77
Copy link
Contributor

dong77 commented Jun 8, 2019

@Brechtpd can we add a totalDowntime per exchange. Maybe also have a view method called

getTotalDowntimeBips to return totalDowntime * 100 / totalExchangeLifetime, this will be a good indicator of the exchange's quality of service.

In future versions, we may want to give a different downtime price per minute, for example:

function getDowntimePricePerMinute() {
  uint multipler = getTotalDowntimeBips() / 100;
  if (totalDowntimeMin < 30 days || multipler == 0) {
    multipler = 1;
  } else if (multipler > 5) {
    multipler = 5;
  }
   return downtimePriceLRCPerMinute * multipler;
}

** UPDATE **

After a second thought, maybe we should still use constant price for the downtime. We should not set up any barrier for a DEX to operate? @Brechtpd ?

@dong77 dong77 self-requested a review June 8, 2019 23:00
@Brechtpd
Copy link
Contributor Author

Brechtpd commented Jun 9, 2019

@Brechtpd can we add a totalDowntime per exchange. Maybe also have a view method called

getTotalDowntimeBips to return totalDowntime * 100 / totalExchangeLifetime, this will be a good indicator of the exchange's quality of service.

Good idea. I exposed getTotalTimeInMaintenanceSeconds and getExchangeCreationTimestamp which allows doing this.

@dong77
Copy link
Contributor

dong77 commented Jun 9, 2019

LGTM.

@dong77
Copy link
Contributor

dong77 commented Jun 9, 2019

please merge once tests pass.

@Brechtpd
Copy link
Contributor Author

After a second thought, maybe we should still use constant price for the downtime. We should not set up any barrier for a DEX to operate? @Brechtpd ?

Maintenance mode needs to be expensive because users cannot withdraw anything in maintenance. If it's cheap the exchange can hold user funds hostage for a long time. Making maintenance mode even more expensive if it's used a lot could make sense, other parameters like total user funds stored in the exchange, ... could also be used.

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