Release: Deploy GoodDaoHouses to development-celo - #300
Open
blueogin wants to merge 13 commits into
Open
Conversation
- Introduced a new deployment script for the GoodDaoHouses governance contract. - Updated deploy-settings.json to include configuration for GoodDaoHouses, including admin and committee roles, as well as minimum stake requirements. - Added GoodDaoHouses address to deployment.json for network integration.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider making
viaGuardiansconfigurable (e.g., via environment or settings) instead of hardcodingfalse, and update the log message inwireFlowSplitterto reflect whether the Safe or guardian path is actually being used. - The error handling in
wireFlowSplitterassumesehas amessageproperty; it would be safer to normalize the error (e.g., viainstanceof ErrororString(e)) to avoid runtime issues with non-Error throws. - For
wireFlowSplitter, adding explicit types forrelease,settings, androot(rather than leaving them implicitlyany) would improve type safety and make the expected shape of these objects clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making `viaGuardians` configurable (e.g., via environment or settings) instead of hardcoding `false`, and update the log message in `wireFlowSplitter` to reflect whether the Safe or guardian path is actually being used.
- The error handling in `wireFlowSplitter` assumes `e` has a `message` property; it would be safer to normalize the error (e.g., via `instanceof Error` or `String(e)`) to avoid runtime issues with non-Error throws.
- For `wireFlowSplitter`, adding explicit types for `release`, `settings`, and `root` (rather than leaving them implicitly `any`) would improve type safety and make the expected shape of these objects clearer.
## Individual Comments
### Comment 1
<location path="scripts/multichain-deploy/9_gooddaohouses-deploy.ts" line_range="189-192" />
<code_context>
+ root
+ );
+ }
+ } catch (e) {
+ console.error("proposal execution failed...", e.message);
+ }
+};
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Broaden error logging to handle non-Error throws and preserve stack/context.
Since `e` may not always be an `Error` instance, accessing `e.message` can both strip useful context and potentially throw. Log the full value instead (e.g. `console.error("proposal execution failed...", e);`) or use structured logging that preserves stack and metadata.
```suggestion
} catch (e) {
// Log the full thrown value to preserve stack/context and avoid assuming Error shape
console.error("proposal execution failed...", e);
}
};
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds deployment support for the GoodDaoHouses governance contract on multichain environments and records the already-deployed development-celo address in the release artifacts.
Changes:
- Added
9_gooddaohouses-deploy.tsto deployGoodDaoHousesas a deterministic UUPS proxy viaProxyFactory1967, with optional FlowSplitter wiring. - Added default
gooddaohousesconfiguration (admin/committee fallbacks, minimum stake thresholds, FlowSplitter placeholders) todeploy-settings.json. - Recorded the
development-celoGoodDaoHousesdeployment address indeployment.json.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scripts/multichain-deploy/9_gooddaohouses-deploy.ts | New deploy script for deterministic UUPS proxy deployment + optional FlowSplitter configuration. |
| releases/deployment.json | Records GoodDaoHouses address for development-celo. |
| releases/deploy-settings.json | Adds default gooddaohouses deployment settings (stakes + FlowSplitter placeholders). |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- Removed the initial whitelisting call for citizenOne and added it back before the voting window. - Ensured proper setup for testing the voting process in GoodDaoHouses.
- Updated the deploySuperGoodDollar function to use clearer variable names for logic and proxy addresses. - Enhanced initialization of NFT proxies to ensure correct contract interactions. - Streamlined the deployment process for better readability and maintainability. - Adjusted test setup in SuperGoodDollar tests for consistency and clarity. - Added error handling in BuyGDClone test to manage network reset failures gracefully.
Collaborator
Author
|
@sirpy |
- Removed try-catch block for network reset in BuyGDClone tests to streamline the setup process. - Directly called networkHelpers.reset with CELO_MAINNET_RPC for improved clarity and efficiency.
…retrieval - Updated CELO_MAINNET_RPC to use an environment variable for flexibility. - Added getCeloForkBlock function to dynamically determine the fork block number, improving test reliability. - Adjusted network reset call in tests to incorporate the new fork block logic.
11 tasks
- Updated the undelegate call in the propose test to handle potential rejections gracefully by using a catch block. - This change enhances the robustness of the test by preventing unhandled promise rejections during execution.
- Added a catch block to the undelegate call in the propose test to gracefully handle potential rejections. - This improvement increases the robustness of the test by preventing unhandled promise rejections during execution.
- Modified the getCeloForkBlock function to return the latest block number minus 5 instead of 50, improving the accuracy of the test environment setup.
- Added flowSplitter and flowSplitterPoolId to deploy-settings.json for GoodDaoHouses. - Updated deployment.json to include GoodDaoHousesFlowSplitter and GoodDaoHousesPoolId. - Improved releaser script to handle FlowSplitter pool creation and configuration dynamically. - Enhanced error handling in the multichain deployment script for better robustness.
Collaborator
Author
|
@sirpy |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
9_gooddaohouses-deploy.tsfor the GoodDaoHouses UUPS proxy (deterministic ProxyFactory deploy + optional FlowSplitter wiring).gooddaohousessettings indeploy-settings.json(admin/committee fallbacks, min stakes, FlowSplitter placeholders).development-celoaddress:GoodDaoHouses=0x4Bc3Cdc036f21b68E034C0f1d90775fc3D725735.Notes
flowSplitter/flowSplitterPoolIdleft unset); houses-only deploy.gooddaohouses.committeeand the production deployer key.About # (link your issue here)
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist:
Summary by Sourcery
Add multichain deployment support for the GoodDaoHouses governance contract and register its development-celo deployment.
New Features:
Enhancements: