Record and provide more information on groups#1567
Merged
Conversation
This function does a lot of storage operations, so the overhead of a cross-contract call is insignificant.
Not ideal, but necessary so stub deployments don't OOG
Rename `terminatedGroups` to `activeTerminatedGroups` to reflect the fact that expired groups get removed from it. Pack group termination status in the `Group` struct itself by reducing blockheight size from 256 to 64 bits. Add `groupIndices` mapping the group public key to its index (flagged with the most significant bit to distinguish the index 0) to avoid O(N) loops when looking for the index of a group identified by the group pubkey.
Instead of deleting members when they withdraw rewards, a member is flagged as having withdrawn their rewards. This requires that all rewards for the same operator be withdrawn at once, so the contract now iterates through all members and sums rewards for each instance of the same operator address.
Also remove the redundancy in `membersOf/getGroupMembers`.
Contributor
Author
|
The failing tests are intermittent errors, afaik unrelated to the changes in this PR. |
pdyraga
reviewed
Apr 10, 2020
Setting it public was done previously to reduce the contract size which is not relevant anymore.
Contributor
Author
|
@pdyraga comments addressed and ready for final review. |
pdyraga
reviewed
Apr 16, 2020
| return memberRewards.mul(memberCount); | ||
| } | ||
|
|
||
| function withdrawableRewards( |
Member
There was a problem hiding this comment.
We should add docs to awaitingRewards and withdrawableRewards.
| // Register new group and request new entry so we can expire the previous two groups | ||
| await operatorContract.registerNewGroup(group3) | ||
| await serviceContract.methods['requestRelayEntry()']({value: entryFeeEstimate, from: requestor}) | ||
| let beneficiary2balance = web3.utils.toBN(await web3.eth.getBalance(beneficiary2)) |
pdyraga
approved these changes
Apr 16, 2020
Member
pdyraga
left a comment
pdyraga
left a comment
There was a problem hiding this comment.
Two non-blocking comments, we can address them in a separate PR.
r-czajkowski
added a commit
that referenced
this pull request
Apr 17, 2020
Adjust the dapp to changes from #1567.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs: #1602
We can't implement external rewards for beacon signers if we erase records of expired groups or delete members when they withdraw rewards. This PR packs an additional
terminatedflag in each group's information, and changes the reward withdrawal logic to get all rewards for the named operator and flagging that operator as withdrawn.To prevent stub contract deployments from running out of gas, the operator contract and its libraries have been refactored slightly.