Feat/operators #28
Feat/operators #28
Conversation
…orizon-accounting-extension
…orizon-accounting-extension
| revert HorizonAccountingExtension_UnauthorizedOperator(); | ||
| } | ||
| _bonder = _operator; | ||
| } |
| @@ -241,54 +274,40 @@ contract HorizonAccountingExtension is Validator, IHorizonAccountingExtension { | |||
| }); | |||
| } | |||
|
|
|||
| /// @inheritdoc IHorizonAccountingExtension | |||
There was a problem hiding this comment.
It's only called by the BondEscalationResolutionModule (which we don't use)
| // [ ] 5. Bonder or operator should be able to remove the operator | ||
| // [ ] 6. Can a bonder have an operator and mix calls by the operator and themselves? | ||
| // [ ] 7. Can a bonder that has an operator be an operator for another address? | ||
| mapping(address _operator => address _bonder) public operators; |
There was a problem hiding this comment.
The mapping returns bonders, not operators. Rename to operatedBonders maybe.
| // RULES: | ||
| // [X] 1. Operator can only operate for one bonder at a time |
There was a problem hiding this comment.
This rule is both limiting and conflicting, as an operator cannot act on behalf of many bonders at the same time even though he may be allowed as such per HorizonStaking.
There was a problem hiding this comment.
It would mix all of the pledges otherwise and would be impossible to pay/slash the bonders. Do you think there could be a way to fix it?
There was a problem hiding this comment.
Problem would be that we wouldn't be able to retrieve on behalf of which bonder an operator is acting, unless we modified a bunch of Prophet's interfaces. No easy way out.
| // TODO: To calculate the amount of pledges, we need to check for both | ||
| // the bonder and the operator. How should we do this? |
There was a problem hiding this comment.
Take into account that a bonder may have set many operators to act on behalf of him.
| // [ ] 6. Can a bonder have an operator and mix calls by the operator and themselves? | ||
| // [ ] 7. Can a bonder that has an operator be an operator for another address? |
There was a problem hiding this comment.
// [ ] 8. Can an operator be a bonder?
// [ ] 9. Can an operator that is a bonder have an operator?
// [ ] 10. Can an operator that is a bonder have an operator that is a bonder?
All these last points may happen desiredly, so they should be carefully tested.
| function _getBonder(address _caller) internal view returns (address _bonder) { | ||
| address _operator = operators[_caller]; | ||
| if(_operator == address(0)) { | ||
| _bonder = _caller; | ||
| } else { | ||
| if(!HORIZON_STAKING.isAuthorized(_caller, _operator, address(this))) { | ||
| revert HorizonAccountingExtension_UnauthorizedOperator(); | ||
| } | ||
| _bonder = _operator; |
There was a problem hiding this comment.
What if a bonder is also an operator but wants to act on his own behalf? There's no way to distinguish this in the current implementation, but there's a precedence of the operator role over the bonder one.
| // TODO: Pledgers holds either the bonder or the operator. | ||
| mapping(bytes32 _disputeId => EnumerableSet.AddressSet _pledger) internal _pledgers; |
There was a problem hiding this comment.
It would be great if we didn't need to store the operators, as they aren't going to be the ones potentially getting slashed.
There was a problem hiding this comment.
The problem is that the amount of pledges it stored in the operators name and not the bonder. To slash the bonder first we need to get the amount of pledges the operator made
There was a problem hiding this comment.
Why isn't BondEscalationAccounting the one tracking pledgesForDispute and pledgesAgainstDispute in the first place, instead of BondEscalationModule?
TODO: To calculate the amount of pledges, we need to check for both the bonder and the operator. How should we do this?
The above is my answer. By implementing it, we could get rid of bonderForRequest, bonderForDispute, and HorizonAccountingExtension_BonderMismatch.
| // Check if the user is actually slashable | ||
| _slashAmount = _calculateSlashAmount(_user, _result, _status); | ||
| _slashAmount = _calculateSlashAmount(_pledger, _result, _status); |
There was a problem hiding this comment.
The _pledger may hold an operator whose address you shouldn't use to calculate the _slashAmount.
There was a problem hiding this comment.
The pledge is made by the operator so it's stored for them in the BondEscalationModule
| function operateFor(address _bonder) external { | ||
| // TODO: Include a function to remove the operator callable by the operator or the bonder | ||
| if(!HORIZON_STAKING.isAuthorized(msg.sender, _bonder, address(this))) { | ||
| revert HorizonAccountingExtension_UnauthorizedOperator(); | ||
| } | ||
| operators[msg.sender] = _bonder; |
There was a problem hiding this comment.
Letting operators call this as is introduces the hazard of a race condition during the execution sequence for a same bonder; forbid sluggish authorized operators to interact.
TODO: Include a function to remove the operator callable by the operator or the bonder
I don't think such function is useful: a set operator may already change his bonder at any time if authorized by another, and if not, an idle operator doesn't hurt the bonder, who may have more operators set.
| // TODO: Pledgers holds either the bonder or the operator. | ||
| mapping(bytes32 _disputeId => EnumerableSet.AddressSet _pledger) internal _pledgers; |
There was a problem hiding this comment.
Why isn't BondEscalationAccounting the one tracking pledgesForDispute and pledgesAgainstDispute in the first place, instead of BondEscalationModule?
TODO: To calculate the amount of pledges, we need to check for both the bonder and the operator. How should we do this?
The above is my answer. By implementing it, we could get rid of bonderForRequest, bonderForDispute, and HorizonAccountingExtension_BonderMismatch.
| ) external onlyAllowedModule(_requestId) onlyParticipant(_requestId, _payer) onlyParticipant(_requestId, _receiver) { | ||
| // TODO: Validate participants against bonders |
There was a problem hiding this comment.
Why? The participants registered in the Oracle must be the requester, proposer and disputer, who may not be bonders.
🤖 Linear
Closes GRT-XXX