-
Notifications
You must be signed in to change notification settings - Fork 369
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
Implement sending of allocated validator payments #11197
Changes from 16 commits
78cc1a6
cb6eded
0813287
a853665
ee1eb46
ff0c20b
4d73b88
54a1037
80873ba
195e718
e1beb8f
771bdfb
07839eb
8889bf3
78c5303
e0493cf
1cb64bf
7da47c6
624e348
2a29cd1
3df09eb
1a50a3f
b47fd27
34d8949
85f9957
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -8,6 +8,7 @@ import "./interfaces/IOracle.sol"; | |||||
import "./interfaces/IStableToken.sol"; | ||||||
import "../common/UsingRegistry.sol"; | ||||||
|
||||||
import "../../contracts/common/FixidityLib.sol"; | ||||||
import "../../contracts/common/Initializable.sol"; | ||||||
import "../../contracts/common/interfaces/IEpochManager.sol"; | ||||||
import "../../contracts/common/interfaces/ICeloVersionedContract.sol"; | ||||||
|
@@ -19,6 +20,8 @@ contract EpochManager is | |||||
ReentrancyGuard, | ||||||
ICeloVersionedContract | ||||||
{ | ||||||
using FixidityLib for FixidityLib.Fraction; | ||||||
|
||||||
struct Epoch { | ||||||
uint256 firstBlock; | ||||||
uint256 lastBlock; | ||||||
|
@@ -73,6 +76,20 @@ contract EpochManager is | |||||
*/ | ||||||
event EpochProcessingEnded(uint256 indexed epochNumber); | ||||||
|
||||||
/** | ||||||
* @notice Emitted when an epoch payment is sent. | ||||||
* @param validator Address of the validator. | ||||||
* @param validatorPayment Amount of cUSD sent to the validator. | ||||||
* @param group Address of the validator's group. | ||||||
* @param groupPayment Amount of cUSD sent to the group. | ||||||
*/ | ||||||
event ValidatorEpochPaymentDistributed( | ||||||
address indexed validator, | ||||||
uint256 validatorPayment, | ||||||
address indexed group, | ||||||
uint256 groupPayment | ||||||
); | ||||||
|
||||||
modifier onlyEpochManagerInitializer() { | ||||||
require(msg.sender == epochManagerInitializer, "msg.sender is not Initializer"); | ||||||
_; | ||||||
|
@@ -319,4 +336,42 @@ contract EpochManager is | |||||
CELOequivalent | ||||||
); | ||||||
} | ||||||
|
||||||
/** | ||||||
* @notice Sends the allocated epoch payment to a validator, their group, and | ||||||
* delegation beneficiary. | ||||||
* @param validator Account of the validator. | ||||||
*/ | ||||||
function sendValidatorPayment(address validator) external { | ||||||
IAccounts accounts = IAccounts(getAccounts()); | ||||||
address signer = accounts.getValidatorSigner(validator); | ||||||
|
||||||
FixidityLib.Fraction memory totalPayment = FixidityLib.newFixed( | ||||||
validatorPendingPayments[signer] | ||||||
); | ||||||
|
||||||
IValidators validators = getValidators(); | ||||||
address group = validators.getValidatorsGroup(validator); | ||||||
(, uint256 commissionUnwrapped, , , , , ) = validators.getValidatorGroup(group); | ||||||
|
||||||
uint256 groupPayment = totalPayment.multiply(FixidityLib.wrap(commissionUnwrapped)).fromFixed(); | ||||||
FixidityLib.Fraction memory remainingPayment = FixidityLib.newFixed( | ||||||
totalPayment.fromFixed() - groupPayment | ||||||
martinvol marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
); | ||||||
(address beneficiary, uint256 fraction) = getAccounts().getPaymentDelegation(validator); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
So it is more clear what does it stand for ? |
||||||
uint256 delegatedPayment = remainingPayment.multiply(FixidityLib.wrap(fraction)).fromFixed(); | ||||||
uint256 validatorPayment = remainingPayment.fromFixed() - delegatedPayment; | ||||||
martinvol marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
IStableToken stableToken = IStableToken(getStableToken()); | ||||||
|
||||||
require(stableToken.transfer(validator, validatorPayment), "mint failed to validator"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not mint but transfer. Can transfer fail? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is validatorPayment always > 0? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If the contract doesn't have enough balance for example. Ideally that should never happen though, if our allocation and minting logic is correct. Auditors in the past have asked to always check return values of e.g. ERC-20 functions that return a success
I guess it could be 0, if delegation and/or commission take up the whole payment. I can add an |
||||||
if (groupPayment > 0) { | ||||||
require(stableToken.transfer(group, groupPayment), "mint failed to validator group"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not mint but transfer |
||||||
} | ||||||
if (delegatedPayment > 0) { | ||||||
require(stableToken.transfer(beneficiary, delegatedPayment), "mint failed to delegatee"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not mint but transfer |
||||||
} | ||||||
|
||||||
emit ValidatorEpochPaymentDistributed(validator, validatorPayment, group, groupPayment); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks broken, even in Validators.sol. We record validator and group payments but not delegatee. Since this is a new event on a new contract, we should correct that |
||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
// SPDX-License-Identifier: UNLICENSED | ||
pragma solidity >=0.8.7 <0.8.20; | ||
|
||
import "../EpochManager.sol"; | ||
|
||
contract EpochManager_WithMocks is EpochManager(true) { | ||
function _setPaymentAllocation(address validator, uint256 amount) external { | ||
validatorPendingPayments[validator] = amount; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
// SPDX-License-Identifier: UNLICENSED | ||
pragma solidity >=0.8.7 <0.8.20; | ||
|
||
import "../../../contracts/common/FixidityLib.sol"; | ||
|
||
contract MockAccounts { | ||
using FixidityLib for FixidityLib.Fraction; | ||
|
||
struct PaymentDelegation { | ||
// Address that should receive a fraction of validator payments. | ||
address beneficiary; | ||
// Fraction of payment to delegate to `beneficiary`. | ||
FixidityLib.Fraction fraction; | ||
} | ||
|
||
mapping(address => PaymentDelegation) delegations; | ||
mapping(address => address) accountToSigner; | ||
|
||
function setPaymentDelegationFor( | ||
address validator, | ||
address beneficiary, | ||
uint256 fraction | ||
) public { | ||
delegations[validator] = PaymentDelegation(beneficiary, FixidityLib.wrap(fraction)); | ||
} | ||
|
||
function deletePaymentDelegationFor(address validator) public { | ||
delete delegations[validator]; | ||
} | ||
|
||
function getPaymentDelegation(address account) external view returns (address, uint256) { | ||
PaymentDelegation storage delegation = delegations[account]; | ||
return (delegation.beneficiary, delegation.fraction.unwrap()); | ||
} | ||
|
||
function setValidatorSigner(address account, address signer) external { | ||
accountToSigner[account] = signer; | ||
} | ||
|
||
function getValidatorSigner(address account) external returns (address) { | ||
return accountToSigner[account]; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
pragma solidity >=0.8.7 <0.8.20; | ||
|
||
interface IMockValidators { | ||
function isValidator(address) external returns (bool); | ||
function isValidatorGroup(address) external returns (bool); | ||
|
||
function updateEcdsaPublicKey(address, address, bytes calldata) external returns (bool); | ||
|
||
function updatePublicKeys( | ||
address, | ||
address, | ||
bytes calldata, | ||
bytes calldata, | ||
bytes calldata | ||
) external returns (bool); | ||
|
||
function setValidator(address) external; | ||
|
||
function setValidatorGroup(address group) external; | ||
|
||
function affiliate(address group) external returns (bool); | ||
|
||
function setDoesNotMeetAccountLockedGoldRequirements(address account) external; | ||
|
||
function setNumRegisteredValidators(uint256 value) external; | ||
|
||
function setMembers(address group, address[] calldata _members) external; | ||
|
||
function setCommission(address group, uint256 commission) external; | ||
|
||
function setAccountLockedGoldRequirement(address account, uint256 value) external; | ||
|
||
function halveSlashingMultiplier(address) external; | ||
|
||
function forceDeaffiliateIfValidator(address validator) external; | ||
|
||
function getTopGroupValidators(address group, uint256 n) external view returns (address[] memory); | ||
|
||
function getValidatorGroup( | ||
address | ||
) | ||
external | ||
view | ||
returns (address[] memory, uint256, uint256, uint256, uint256[] memory, uint256, uint256); | ||
|
||
function getValidatorGroupSlashingMultiplier(address) external view returns (uint256); | ||
|
||
function meetsAccountLockedGoldRequirements(address account) external view returns (bool); | ||
|
||
function getNumRegisteredValidators() external view returns (uint256); | ||
|
||
function getAccountLockedGoldRequirement(address account) external view returns (uint256); | ||
|
||
function calculateGroupEpochScore(uint256[] calldata uptimes) external view returns (uint256); | ||
|
||
function getGroupsNumMembers(address[] calldata groups) external view returns (uint256[] memory); | ||
|
||
function groupMembershipInEpoch(address addr, uint256, uint256) external view returns (address); | ||
|
||
function getGroupNumMembers(address group) external view returns (uint256); | ||
} |
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 believe we didn't want to use signers @mcortesi @martinvol
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 was basing this on how the current code works:
elected
array in EpochManager gets set by callingElection.electNValidatorSigners
Validators.getTopGroupValidators
So in the current state, the correct way to index into
elected
is via signer addresses.If we want to change this logic, we should thoroughly double check everything is returning/using the correct type of validator address. As mentioned on Slack, a similar issue exists with the current use of
Validators.computeEpochReward
, which expects a validator account, but gets a signer address (generated by the election). I would suggest this be done in a follow-up issue/PR that covers all instances of this.In general I agree, we can probably just use account addresses everywhere. If I understand correctly, the only reason we used signer addresses in some places was because that's what celo-blockchain cared about in the PoS logic.
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.
yes, we don't want signeers. EpochManager will have a list of elected account (not signers) and we should change elections to not give signers, i believe we even have a draft methos doing that...
the client didn't care about account, only about signers and so it sent signers everywhere, but that's unnecessary complexity now
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.
Created an issue to track progress on this
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.
@mcortesi, sounds great, that's how I understood things.
As mentioned, I would merge this PR as is, so it's consistent with the current state of the logic, and remove signers in the PR that will fix @soloseng's issue.
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.
PR making the changes.