-
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
Implement sending of allocated validator payments #11197
Conversation
*/ | ||
function sendValidatorPayment(address validator) external { | ||
IAccounts accounts = IAccounts(getAccounts()); | ||
address signer = accounts.getValidatorSigner(validator); |
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:
- the
elected
array in EpochManager gets set by callingElection.electNValidatorSigners
- That functions gets validator addresses via
Validators.getTopGroupValidators
- Which grabs the validator accounts and translates them to validator signer addresses, returning those
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.
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.
|
||
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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Can transfer fail?
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 bool
, as good practice.
is validatorPayment always > 0?
I guess it could be 0, if delegation and/or commission take up the whole payment. I can add an if(non-zero)
around this as well. That said, transfers of 0 do not fail, as evidenced by the test_doesNothingIfNotAllocated
test not reverting.
require(stableToken.transfer(beneficiary, delegatedPayment), "mint failed to delegatee"); | ||
} | ||
|
||
emit ValidatorEpochPaymentDistributed(validator, validatorPayment, group, groupPayment); |
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.
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
|
||
require(stableToken.transfer(validator, validatorPayment), "mint failed to validator"); | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
not mint but transfer
require(stableToken.transfer(group, groupPayment), "mint failed to validator group"); | ||
} | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
not mint but transfer
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.
🚀
FixidityLib.Fraction memory remainingPayment = FixidityLib.newFixed( | ||
totalPayment.fromFixed() - groupPayment | ||
); | ||
(address beneficiary, uint256 fraction) = getAccounts().getPaymentDelegation(validator); |
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.
(address beneficiary, uint256 fraction) = getAccounts().getPaymentDelegation(validator); | |
(address beneficiary, uint256 delegationFraction) = getAccounts().getPaymentDelegation(validator); |
So it is more clear what does it stand for ?
Description
Implements
sendValidatorPayment(address validator)
, which sends the cUSD validator payment allocated viaallocateValidatorRewards
to the validator, group, and delegation beneficiary.TODO:
distributeEpochPaymentsFromSigner
need to be carried overcomputeEpochReward
✅Questions:
Follow-ups:
Other changes
Changes to various mock contracts. In particular:
Tested
Forge unit tests.
Related issues
Documentation
Should add docs for validators on how to call this function to claim their payments.