-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add allow multiple setScores calls #33
Conversation
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.
How are the change proposed here going to interact with the validation step proposed in #32? I think it's important to run the validation only after we collected all scores, right?
Validation has to be moved to |
} | ||
|
||
function reward( | ||
address payable[] memory addresses, | ||
uint64[] memory scores | ||
) private { | ||
validateScores(scores); |
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.
Validation has to be moved to
reward()
then, where it gets access to all scores
I think this is a good minimal version.
I think it would be better to run the validation on combined scores as part of setScores()
call too, so that we can catch the problem sooner.
In the current version:
- First call to
setScores()
sets adds0.5
- Second call to
setScores()
sets adds0.9
- no problem noticed, although we are already over1.0
- Second call to
setScores()
sets total0.1
and now the error is reported?
The trick is to run validateScores
on the combination of older scores with the currently reported scores.
Thoughts?
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.
I don't think checking earlier is an improvement. The flow is as follows: At the end of each round, the evaluate service will calculate all scores. Afterwards, it batches the scores and then calls setScores
multiple times. From that service's perspective, there is no measurable benefit in aborting early. Also, if you abort early, there still is no way to recover, since you already started submitting scores. I think if we have a scores overflow, we will just not pay any rewards for this round.
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.
I'm going to merge for now, if you disagree let's open an issue for this
This allows
setScores
to be called multiple times per round, which can be required if the participant set = arguments length gets too large.I've also removed
summaryText
, since we don't have a real need for it, and it's not worth the storage discussion.