Skip to content
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

Code cleanup #45

Merged
merged 5 commits into from
Aug 21, 2022
Merged

Code cleanup #45

merged 5 commits into from
Aug 21, 2022

Conversation

doerfli
Copy link
Contributor

@doerfli doerfli commented Aug 19, 2022

No description provided.

@doerfli doerfli marked this pull request as draft August 19, 2022 14:50
@doerfli doerfli marked this pull request as ready for review August 19, 2022 15:25
@@ -276,7 +276,7 @@ contract PoolController is
pool.updatedAt = block.timestamp;
}

function isArchivingAllowed(uint256 riskpoolId) external view returns (bool) {
function isArchivingAllowed(uint256 riskpoolId) external view {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feels strange that an isXYZ function does not return bool.
seems this is only used in

  • ComponentOwnerService.archive(uint256 id)
  • InstanceOperatorService.archive(uint256 id)

in both cases a sequence of 3 requires would feel more natural as the nested if statement.
please consider something like

function archive(...)
...
{
require(comonent_type==riskpool, "ERROR...NOT_RISKPOOL")
require(coponent_sate==..., "ERROR...TRANSITION_TO_ARCHIVED_STATE_INVALID")
require(bundle.unburnedBundles() == 0, "RISKPOOL_HAS_UNBURNT_BUNDLES")
..
}

or a modifier archivingAllowed ... with the same effect

Copy link
Contributor Author

@doerfli doerfli Aug 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought when looking at the code. But it seems i forgot to fix it ;-)
But the if is still required as the require is not for all components, but only archive.
Also not having it in poolcontroller would require adding a dependency to bundlecontroller to both ComponentOwnerService and InstanceOperatorService for checking the unburnedBundles. I also don't like that approach. maybe just rename the method from isArchivingAllowed to archivePreconditionCheck make its more understandable. Ill do that in a separate PR to be able to close this one now.

@doerfli doerfli merged commit 105f56b into master Aug 21, 2022
@doerfli doerfli deleted the feature/code-cleanup branch August 21, 2022 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants