Follow-up PR for Compensation with Penalties#471
Conversation
derekpierre
left a comment
There was a problem hiding this comment.
Nice work. Still making my way through. Some comments in the meantime.
| address _fundHolder, | ||
| uint256 _startPeriodForRewards | ||
| ) Periods(genesisTime, periodDuration) { | ||
| require(admin != address(0), "Admin required"); |
There was a problem hiding this comment.
Should we also add some other require statements...?:
| require(admin != address(0), "Admin required"); | |
| require(admin != address(0), "Admin required"); | |
| require(_fixedCompensationPerPeriod2Penalties <= _fixedCompensationPerPeriod, "Invalid 2 penalty compensation"); | |
| require(_fixedCompensationPerPeriod3Penalties <= _fixedCompensationPerPeriod2Penalties, "Invalid 3 penalty compensation"); |
There was a problem hiding this comment.
well it's not breaking any math there so not really necessary. I'll add it if you would like
| infraction = deployer.deploy(project.InfractionCollector) | ||
| deployments = [infraction] | ||
| deployer.finalize(deployments=deployments) | ||
|
|
There was a problem hiding this comment.
Need to merge the registry (similar to what is done in the lynx script)?:
| merge_registries( | |
| registry_1_filepath=TAPIR_REGISTRY, | |
| registry_2_filepath=deployer.registry_filepath, | |
| output_filepath=TAPIR_REGISTRY, | |
| ) |
| CONSTRUCTOR_PARAMS_DIR, ARTIFACTS_DIR, | ||
| ) | ||
| from deployment.params import Deployer | ||
|
|
There was a problem hiding this comment.
| from deployment.registry import merge_registries | |
| @pytest.fixture() | ||
| def penalty_board(project, creator, taco_application): | ||
| contract = project.PenaltyBoardForTACoApplicationMock.deploy(sender=creator) | ||
| taco_application.setPenaltyBoard(contract.address, sender=creator) |
There was a problem hiding this comment.
Any reason you don't want to include the setting of the penalty board in the taco_application fixture itself? Then you wouldn't have to include the penalty_board fixture in tests where you aren't making assertions on the penalty board contract? For example, test_request_unstake, test_release, test_child_sync.
i.e. you can potentially do the following instead:
def taco_application(..., pentalty_board):
...
taco_application.setPenaltyBoard(contract.address, sender=creator)
def penalty_board(project, creator):
contract = project.PenaltyBoardForTACoApplicationMock.deploy(sender=creator)
return contract
| penalty_board.addPenalizedProvidersForPeriod(provs, current, sender=informer) | ||
| assert penalty_board.getPenalizedPeriodsByStaker(other_account.address) == [current] | ||
|
|
||
| # Advance into period 1; then both 2 and 3 are valid. |
There was a problem hiding this comment.
Is it period 1 or 3 for the comment?
| # Advance into period 1; then both 2 and 3 are valid. | |
| # Advance into period 3; then both 2 and 3 are valid. |
| mock_taco_app.setRoles( | ||
| stakeless_provider.address, | ||
| owner.address, | ||
| beneficiary.address, |
There was a problem hiding this comment.
Can a stakeless node have a beneficiary?
Latest TACoApplication, with required API changes, lives on a parallel universe. Waiting for rebase.
Type of PR:
Required reviews:
What this does:
This PR extends
PenaltyBoardwith a fixed compensation model for stakers:setPenalizedProvidersForPeriodappends the period index to each provider’s list; there is no on-chain period→providers mapping (subgraph can provide that view). This is a departure from the original PR Periods and PenaltyBoard contracts #456, where storage was period-centric.TACoApplicationfor roles and beneficiary.getAccruedBalanceand persisted onwithdraw).[startPeriod - PENALTY_WINDOW_PERIODS, current].numPeriods × fixedCompensationPerPeriod.kin that range falls in[P - window, P]; if so, that period gets zero, else full.Issues fixed/closed:
Why it's needed:
Notes for reviewers: