add: update ipfs by manager + new registerapp #302
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @sirpy - I've reviewed your changes - here's some feedback:
- Clean up the commented-out
host.registerAppcode in GoodCollectiveSuperApp to avoid confusion and centralize super app registration in the factories. - Verify that replacing DEFAULT_ADMIN_ROLE with MANAGER_ROLE for pool control aligns with your intended permission model and that MANAGER_ROLE is granted correctly on pool creation.
- Consider adding error handling or logging around the
registerAppcalls in the factories to catch and surface any host registration failures during deployment.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Clean up the commented-out `host.registerApp` code in GoodCollectiveSuperApp to avoid confusion and centralize super app registration in the factories.
- Verify that replacing DEFAULT_ADMIN_ROLE with MANAGER_ROLE for pool control aligns with your intended permission model and that MANAGER_ROLE is granted correctly on pool creation.
- Consider adding error handling or logging around the `registerApp` calls in the factories to catch and surface any host registration failures during deployment.
## Individual Comments
### Comment 1
<location> `packages/contracts/contracts/UBI/UBIPoolFactory.sol:21` </location>
<code_context>
contract DirectPaymentsFactory is AccessControlUpgradeable, UUPSUpgradeable {
error NOT_PROJECT_OWNER();
+ error NOT_PROJECT_MANAGER();
error NOT_POOL();
</code_context>
<issue_to_address>
Review error usage for consistency with new access control.
Update all revert statements and error handling to use NOT_PROJECT_MANAGER where appropriate, and remove outdated errors that are no longer relevant to the new access control logic.
Suggested implementation:
```
error NOT_PROJECT_MANAGER();
error NOT_POOL();
```
```
if (msg.sender != projectManager) {
revert NOT_PROJECT_MANAGER();
}
```
```
require(hasRole(PROJECT_MANAGER_ROLE, msg.sender), "Not project manager");
```
```
if (!isPool(msg.sender)) {
revert NOT_POOL();
}
```
- You will need to update any references to `projectOwner` to `projectManager` if the access control logic has changed.
- Ensure that the role constants (`PROJECT_MANAGER_ROLE`) are defined and used consistently throughout the contract.
- Remove any other outdated error definitions and usages related to project ownership that are no longer relevant.
- Review the rest of the contract for any additional revert statements or require checks that should be updated to use the new error and access control logic.
</issue_to_address>
### Comment 2
<location> `packages/contracts/contracts/GoodCollective/GoodCollectiveSuperApp.sol:93` </location>
<code_context>
// Set the super token address
superToken = _superToken;
- // Define the callback definitions for the app
- uint256 callBackDefinitions = SuperAppDefinitions.APP_LEVEL_FINAL;
+ // // Define the callback definitions for the app
+ // uint256 callBackDefinitions = SuperAppDefinitions.APP_LEVEL_FINAL;
</code_context>
<issue_to_address>
Commented-out registration logic may cause confusion.
If registration is managed elsewhere, please remove these commented lines or add a brief note clarifying the change.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| contract UBIPoolFactory is AccessControlUpgradeable, UUPSUpgradeable { | ||
| error NOT_PROJECT_OWNER(); | ||
| error NOT_PROJECT_MANAGER(); |
There was a problem hiding this comment.
suggestion: Review error usage for consistency with new access control.
Update all revert statements and error handling to use NOT_PROJECT_MANAGER where appropriate, and remove outdated errors that are no longer relevant to the new access control logic.
Suggested implementation:
error NOT_PROJECT_MANAGER();
error NOT_POOL();
if (msg.sender != projectManager) {
revert NOT_PROJECT_MANAGER();
}
require(hasRole(PROJECT_MANAGER_ROLE, msg.sender), "Not project manager");
if (!isPool(msg.sender)) {
revert NOT_POOL();
}
- You will need to update any references to
projectOwnertoprojectManagerif the access control logic has changed. - Ensure that the role constants (
PROJECT_MANAGER_ROLE) are defined and used consistently throughout the contract. - Remove any other outdated error definitions and usages related to project ownership that are no longer relevant.
- Review the rest of the contract for any additional revert statements or require checks that should be updated to use the new error and access control logic.
| // Set the super token address | ||
| superToken = _superToken; | ||
|
|
||
| // Define the callback definitions for the app |
There was a problem hiding this comment.
nitpick: Commented-out registration logic may cause confusion.
If registration is managed elsewhere, please remove these commented lines or add a brief note clarifying the change.
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Excessive Gas Price Increase ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| packages/contracts/hardhat.config.ts | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| chainId: 42220, | ||
| url: `https://forno.celo.org`, | ||
| gasPrice: 5000000000, | ||
| gasPrice: 25.1e9, |
There was a problem hiding this comment.
Excessive Gas Price Increase 
Tell me more
What is the issue?
The gas price modification from 5000000000 to 25.1e9 for Celo networks represents a significant increase (about 5x) which could lead to unnecessarily expensive transactions.
Why this matters
Higher gas prices will result in increased transaction costs for users, potentially making the system prohibitively expensive to use. The original gas price was already sufficient for most Celo transactions.
Suggested change ∙ Feature Preview
Maintain the previous gas price or use a more moderate increase. Example:
gasPrice: 10000000000, // 10 gweiProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
add: update ipfs by manager + new registerapp
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Description by Korbit AI
What change is being made?
Update IPFS management in the
DirectPaymentsFactoryandUBIPoolFactorycontracts by introducing a new permission check for project managers, add registration of apps with the host usingSuperAppDefinitions, and adjust gas prices in the hardhat configuration for the Celo network.Why are these changes being made?
These changes are being made to improve access control by checking if the sender is a project manager instead of an owner, register super app contracts to comply with Superfluid's requirements and to optimize transaction costs on the Celo network by updating gas prices. Introducing a new interface for app registration enhances the modularity and extensibility of the smart contracts.