Xdc deploy #283
Merged
Xdc deploy #283
Conversation
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
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Blocking issues:
- Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `scripts/multichain-deploy/1_basicdao-deploy.ts:151` </location>
<code_context>
- // console.log("setting identity auth period");
- // await Identity.setAuthenticationPeriod(365).then(printDeploy);
-
- const avatar = await daoCreator.avatar();
+ const avatar = release.Avatar || (await daoCreator.avatar());
let Avatar = new ethers.Contract(
avatar,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using release.Avatar as a fallback may mask deployment issues.
Validating that release.Avatar matches the deployed avatar address will help prevent inconsistencies caused by outdated release files.
```suggestion
const deployedAvatar = await daoCreator.avatar();
if (release.Avatar && release.Avatar.toLowerCase() !== deployedAvatar.toLowerCase()) {
throw new Error(
`Release file Avatar address (${release.Avatar}) does not match deployed avatar address (${deployedAvatar}). Please update the release file or investigate the deployment.`
);
}
let Avatar = new ethers.Contract(
deployedAvatar,
["function owner() view returns (address)", "function nativeToken() view returns (address)"],
["function owner() view returns (address)", "function nativeToken() view returns (address)"],
root
);
```
</issue_to_address>
### Comment 2
<location> `scripts/multichain-deploy/1_basicdao-deploy.ts:163-161` </location>
<code_context>
Avatar.address,
schemes,
schemes.map(_ => ethers.constants.HashZero),
- ["0x0000001f", "0x00000001"],
+ ["0x0000001f", "0x0000001f"],
""
)
</code_context>
<issue_to_address>
**🚨 issue (security):** Changing both permission masks to 0x0000001f may grant excessive permissions.
Please verify that increasing the second scheme's permission mask is intentional and does not introduce security or access issues.
</issue_to_address>
### Comment 3
<location> `scripts/multichain-deploy/1_basicdao-deploy.ts:191` </location>
<code_context>
if (isProduction) {
console.log("renouncing minting rights on production env");
await GoodDollar.renounceMinter().then(printDeploy);
+ const ctrl = await ethers.getContractAt("Controller", await Avatar.owner());
+ console.log("setting identity authentication period to 180 days");
+ await ctrl
</code_context>
<issue_to_address>
**🚨 issue (security):** Directly setting authentication period via genericCall may bypass access checks.
Restrict genericCall usage to authorized entities to prevent privilege escalation.
</issue_to_address>
### Comment 4
<location> `scripts/multichain-deploy/helpers.ts:80-82` </location>
<code_context>
+ proposalActions.map(_ => _[3])
];
-
try {
if (viaGuardians) {
await executeViaSafe(
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Catching initialization errors and logging may mask underlying deployment issues.
Logging errors alone may allow the deployment to proceed in an inconsistent state. Evaluate if stricter error handling or rollback is required to maintain contract integrity.
</issue_to_address>
### Comment 5
<location> `scripts/proposals/gip-25-xdc-upgrade-ubi.ts:53` </location>
<code_context>
+ production: "0x6f252280eB53df085eAD27BBe55d615741A8268D",
+ "production-mainnet": "0x7baFe060A37E31E707b8B28a90a36731ee99aFBa"
+};
+export const upgradeCeloStep1 = async (network, checksOnly) => {
+ let [root] = await ethers.getSigners();
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the three upgrade functions into a single generic runner with a reusable ProposalAction type and helper.
```markdown
You can collapse all three `upgrade*Step1` functions into one generic runner that
1. gathers signer/network setup
2. deploys common contracts (`UBISchemeV2`, `IdentityV3`, …)
3. builds a _named_ actions list
4. executes via guardian/safe
5. runs any post‐simulation checks
This both removes the 400+ lines of near‐identical code and replaces the `[a[0],a[1]…]` tuples with a proper object.
---
### 1) Define a reusable ProposalAction type + helper
```ts
type ProposalAction = {
target: string
signature: string // e.g. "upgradeTo(address)"
args: any[] // e.g. [ubiImpl.address]
value?: string // defaults to "0"
}
function encodeProposal(actions: ProposalAction[]) {
return actions.map(({ target, signature, args, value = "0" }) => ({
target,
signature,
data: ethers.utils.defaultAbiCoder.encode(
signature.match(/\((.*)\)/)![1].split(","),
args
),
value,
}))
}
```
### 2) Centralize the runner
```ts
async function runUpgrade(
network: string,
checksOnly: boolean,
actions: ProposalAction[],
postCheck?: () => Promise<void>
) {
const [root] = await ethers.getSigners()
const isProd = network.includes("production")
if (isProd) verifyProductionSigner(root)
let guardian = root, env = network
if (isSimulation) {
env = network; // or map to your DAO key
guardian = await ethers.getImpersonatedSigner(dao[env].GuardiansSafe)
await root.sendTransaction({
value: ethers.utils.parseEther("1"),
to: dao[env].GuardiansSafe
})
}
const encoded = encodeProposal(actions)
const contracts = encoded.map(x => x.target)
const sigs = encoded.map(x => x.signature)
const datas = encoded.map(x => x.data)
const values = encoded.map(x => x.value)
if (isProd && !checksOnly) {
await executeViaSafe(contracts, values, sigs, datas, dao[env].GuardiansSafe, network)
} else if (!checksOnly) {
await executeViaGuardian(contracts, values, sigs, datas, guardian, env)
}
if (!isProd && postCheck) await postCheck()
}
```
### 3) Rewrite each step in 10–15 lines
```ts
export const upgradeCeloStep1 = async (network, checksOnly) => {
const { UBIScheme, Identity, MpbBridge, GoodDollar, Controller, Avatar } = dao[network]
const ubiImpl = await ethers.deployContract("UBISchemeV2")
const identityImpl= await ethers.deployContract("IdentityV3")
const bridgeImpl = bridgeUpgradeImpl[network]
const upgradeCall = ethers.utils.keccak256(
ethers.utils.toUtf8Bytes("upgrade()")
).slice(0,10)
const gd = await ethers.getContractAt("IGoodDollar", GoodDollar)
const toBurn = (await gd.balanceOf(Avatar)).add(await gd.balanceOf(MpbBridge))
const actions: ProposalAction[] = [
{ target: UBIScheme, signature: "upgradeTo(address)", args: [ubiImpl.address] },
{ target: Identity, signature: "upgradeTo(address)", args: [identityImpl.address] },
{ target: MpbBridge,
signature: "upgradeToAndCall(address,bytes)",
args: [bridgeImpl, upgradeCall]
},
{ target: MpbBridge, signature: "withdraw(address,uint256)", args: [GoodDollar, 0] },
{ target: GoodDollar, signature: "burn(uint256)", args: [toBurn.toString()] },
{ target: Controller,
signature: "registerScheme(address,bytes32,bytes4,address)",
args: [MpbBridge, ethers.constants.HashZero, "0x00000001", Avatar]
}
]
await runUpgrade(network, checksOnly, actions, async () => {
console.log("post‐simulation checks…")
// your existing balance/permission logs here
})
}
```
Repeat the same pattern for `upgradeFuseStep1` and `upgradeEthStep1`, passing only the per‐chain actions and checks. This will:
- Reduce 400+ lines down to ~60
- Avoid deep `[a[0], a[1], …]` array indexing
- Keep all existing behavior intact
- Make future changes much easier to follow
```
</issue_to_address>
### Comment 6
<location> `releases/deployment.json:675` </location>
<code_context>
0xfA2958CB79b0491CC627c1557F441eF849Ca8eb1
</code_context>
<issue_to_address>
**security (generic-api-key):** Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
*Source: gitleaks*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| await daoCreator | ||
| .setSchemes( | ||
| Avatar.address, | ||
| schemes, |
There was a problem hiding this comment.
🚨 issue (security): Changing both permission masks to 0x0000001f may grant excessive permissions.
Please verify that increasing the second scheme's permission mask is intentional and does not introduce security or access issues.
![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 |
|---|---|---|
 |
Inefficient multiple array iterations ▹ view | |
|
Inefficient contract call pattern ▹ view | |
|
Redundant contract instantiation ▹ view | |
 |
Hardcoded network parameter in executeViaSafe calls ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| scripts/multichain-deploy/8_disthelper-deploy.ts | ✅ |
| hardhat.config.ts | ✅ |
| scripts/multichain-deploy/1_basicdao-deploy.ts | ✅ |
| scripts/multichain-deploy/0_proxyFactory-deploy.ts | ✅ |
| scripts/multichain-deploy/helpers.ts | ✅ |
| scripts/proposals/gip-25-xdc-upgrade-ubi.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.
Xdc deploy
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
blueogin
reviewed
Oct 29, 2025
Collaborator
blueogin
left a comment
blueogin
left a comment
There was a problem hiding this comment.
I’m not fully familiar with the full workflow of the project, but from my perspective, it looks good.
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
Description by Korbit AI
What change is being made?
Update to support XDC deployment with multiple related changes across contracts, deploy scripts, configurations, and tests, including enhanced event data for native purchases, smoother UBIScheme activation, updated deployment targets and networks, and minor config/test adjustments.
Why are these changes being made?
Enable smoother cross-chain (XDC) deployments and upgrades, improve reliability and observability of native token purchases, and align deployment tooling and configs with the new XDC environments (including updated addresses and chain-specific behaviors).