feat: [sc-2168] LBPFactoryNoAccessControl#116
Conversation
Removed the onlyOwner() modifier from the function deployLBPManager. This was done because the Celo Safe is flaky so it has been removed from the LBPManager creation flow.
|
This pull request has been linked to Shortcut Story #2168: LBPFactoryNoAccessControl. |
Codecov ReportBase: 63.84% // Head: 61.12% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## development #116 +/- ##
===============================================
- Coverage 63.84% 61.12% -2.73%
===============================================
Files 9 10 +1
Lines 426 445 +19
Branches 106 110 +4
===============================================
Hits 272 272
- Misses 154 173 +19
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
tamagosante
left a comment
tamagosante
left a comment
There was a problem hiding this comment.
Options
- Don't actually do versioning for LBP
- IF do versioning, then need to clean up some exports, .jsons, renaming, and version number
| @@ -16,13 +16,14 @@ pragma solidity 0.8.17; | |||
|
|
|||
There was a problem hiding this comment.
should rename this file too to include "V1"?
There was a problem hiding this comment.
BUT, I'm also fine with maybe not versioning LBP for now
There was a problem hiding this comment.
I did not add the version to the standard LBPManagerFactory because for now, we will only use the other one
| * @dev Governance to create new LBPManager contracts. | ||
| */ | ||
| contract LBPManagerFactory is CloneFactory, Ownable { | ||
| bytes6 public version = "2.1.0"; |
There was a problem hiding this comment.
I expected to see "1.0.0" here, since LBP is still v1?
I also expect LBP and Seed versioning to be independent.
MAYBE we can find a case to tie the versions together, but I doubt that.
| * @title LBPManager Factory | ||
| * @dev Governance to create new LBPManager contracts. | ||
| */ | ||
| contract LBPManagerFactory is CloneFactory, Ownable { |
There was a problem hiding this comment.
| contract LBPManagerFactory is CloneFactory, Ownable { | |
| contract LBPManagerFactoryV1 is CloneFactory, Ownable { |
| @@ -0,0 +1,785 @@ | |||
| { | |||
There was a problem hiding this comment.
this file should be removed for the "V1" one?
| "name": "celo", | ||
| "chainId": "42220", | ||
| "contracts": { | ||
| "LBPManager": { |
There was a problem hiding this comment.
both contracts are the same, aren't they?
2 participants
What has been done?
onlyOwner()modifier fromLBPManagerFactory.deployLBPManager()LBPManagerFactory->LBPManagerFactoryNoAccessControlAdditional Notes
For the reviewer
This branch will get merged into the development branch, not in the main branch. The reason for this is that the code coverage would otherwise be lowered. In this way, we can make sure that the code coverage is 100% before we merge development into main
Front End change
The frontend should pull the ABIs from the development branch once this is merged. Only once we move to production should the FE pull from the main branch
Implemented review comments