Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unnecessary assertion in initialize_epoch #136

Open
djrtwo opened this issue May 10, 2018 · 2 comments
Open

Unnecessary assertion in initialize_epoch #136

djrtwo opened this issue May 10, 2018 · 2 comments

Comments

@djrtwo
Copy link
Contributor

djrtwo commented May 10, 2018

There is an assertion in initialize_epoch when updating reward_factor that I'm pretty sure should not exist. It should either be a conditional assignment or should entirely be removed.

assert self.reward_factor > 0

Such an assertion that is out of the control of the caller via method params should not exist in this protocol level method. If this method fails, then the protocol will not be able to continue functioning.

It appears to me that reward_factor can never be negative because of the logic to assign it:

adj_interest_base: decimal = self.BASE_INTEREST_FACTOR / self.sqrt_of_total_deposits()
self.reward_factor = adj_interest_base + self.BASE_PENALTY_FACTOR * (self.esf() - 2)

Neither adj_interest_base nor self.BASE_PENALTY_FACTOR * (self.esf() - 2) will never be negative. self.BASE_PENALTY_FACTOR * (self.esf() - 2) can equal zero, but self.BASE_INTEREST_FACTOR / self.sqrt_of_total_deposits() will always be greater than zero except in extreme cases where the denominator is extremely large (more ether than exists). So if these are the case, then reward_factor will always be larger than zero.

If we are actually concerned with this value equalling zero in some cases, then we should have logic to recover gracefully rather than throwing with an assert. Something like the following:

if self.reward_factor == 0: 
    self.reward = DEFAULT_LOWEST_REWARD_FACTOR
@ChihChengLiang
Copy link
Contributor

removing that assert make perfect sense. Currently, the assertion is not giving useful information but introducing complexity when tracing bug.

Would also suggest assigning a minimum value for sqrt_of_total_deposits, to prevent self.BASE_INTEREST_FACTOR / self.sqrt_of_total_deposits() being too large.

@djrtwo
Copy link
Contributor Author

djrtwo commented Jun 6, 2018

I'm not sure what a reasonable lower bound on the sqrt_of_total_deposits would be. Can you expand upon your comment @ChihChengLiang ?

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

No branches or pull requests

2 participants