Feature: Bridge GoodDollar using OFT adapter via LayerZero Endpoint v2 #8
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
…gic and update dependencies
…ipient configuration
…contracts by removing bridge limits and minting logic, enhancing clarity and maintainability
![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 9 issues, and left some high level feedback:
- The
GoodDollarOFTAdapterconstructor signature ((address _token, address _lzEndpoint)) does not match how it is instantiated inoft-deploy.ts([tokenAddress, MinterBurner.address, lzEndpoint, owner]), which will cause deployment to fail; the constructor/initializer arguments and deploy script need to be aligned. - In
GoodDollarOFTAdapter._credit, therequestIdis derived usingblock.timestampand local data, which makes it impossible to deterministically know this ID off-chain or on the source chain for callingapproveRequestahead of time; consider passing a request identifier through the LayerZero payload instead of computing it from non-deterministic data. - The
set-minter-burner-limits.tsscript treats the env limits asNumberand compares them directly toweeklyMintLimit/monthlyMintLimitBigNumbers (and uses== undefinedchecks that will never fire becauseNumber(undefined)isNaN), which will lead to incorrect comparisons and skipped updates; parse the env vars as strings and convert toBigNumber(e.g., viaparseEther) before comparison and encoding.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `GoodDollarOFTAdapter` constructor signature (`(address _token, address _lzEndpoint)`) does not match how it is instantiated in `oft-deploy.ts` (`[tokenAddress, MinterBurner.address, lzEndpoint, owner]`), which will cause deployment to fail; the constructor/initializer arguments and deploy script need to be aligned.
- In `GoodDollarOFTAdapter._credit`, the `requestId` is derived using `block.timestamp` and local data, which makes it impossible to deterministically know this ID off-chain or on the source chain for calling `approveRequest` ahead of time; consider passing a request identifier through the LayerZero payload instead of computing it from non-deterministic data.
- The `set-minter-burner-limits.ts` script treats the env limits as `Number` and compares them directly to `weeklyMintLimit`/`monthlyMintLimit` BigNumbers (and uses `== undefined` checks that will never fire because `Number(undefined)` is `NaN`), which will lead to incorrect comparisons and skipped updates; parse the env vars as strings and convert to `BigNumber` (e.g., via `parseEther`) before comparison and encoding.
## Individual Comments
### Comment 1
<location> `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol:346-348` </location>
<code_context>
+ ) internal virtual override returns (uint256 amountReceivedLD) {
+ if (_to == address(0x0)) _to = address(0xdead); // _mint(...) does not support address(0x0)
+
+ // Generate a request ID from the message (using source chain ID and amount as a simple hash)
+ // In a real implementation, you might want to pass this from the LayerZero message
+ uint256 requestId = uint256(keccak256(abi.encode(_srcEid, _to, _amountLD, block.timestamp)));
+
+ // Enforce limits before processing
</code_context>
<issue_to_address>
**issue (bug_risk):** Request ID generation makes pre-approval via `approveRequest` effectively impossible.
Because `requestId` is computed with `block.timestamp` inside `_credit` on delivery, callers can’t know it in advance and thus can’t realistically call `approveRequest(requestId)` beforehand. To support pre-approval by an off-chain approver, the ID needs to be deterministically derived from data available before send (e.g., source chain nonce/tx hash, src/dst/amount) and/or carried through the LayerZero payload instead of relying on `block.timestamp`.
</issue_to_address>
### Comment 2
<location> `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol:355-362` </location>
<code_context>
+ // For approved requests, call approveRequest() with the requestId before the transfer
+ _enforceLimits(_to, _to, _amountLD, requestId);
+
+ // Calculate fee if fee recipient is set and fee is configured
+ uint256 fee = 0;
+ if (feeRecipient != address(0) && bridgeFees.fee > 0) {
+ fee = _takeFee(_amountLD);
+ }
+
+ // Mint tokens to recipient (amount minus fee)
+ uint256 amountToRecipient = _amountLD - fee;
+ minterBurner.mint(_to, amountToRecipient);
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Fee calculation ignores min/max bounds and can revert on small amounts.
`BridgeFees` exposes `minFee`/`maxFee`, but `_takeFee` returns only the basis-point fee and `_credit` uses `_amountLD - fee` directly, so those bounds are never applied. Consider clamping `fee` into [`minFee`, `maxFee`] and ensuring `fee < _amountLD` (or rejecting transfers below a minimum amount) to both respect the configured bounds and avoid underflow on small transfers.
Suggested implementation:
```
// For approved requests, call approveRequest() with the requestId before the transfer
_enforceLimits(_to, _to, _amountLD, requestId);
// Calculate fee if fee recipient is set and fee is configured
uint256 fee = 0;
if (feeRecipient != address(0) && bridgeFees.fee > 0) {
// Base fee from basis points
fee = _takeFee(_amountLD);
// Apply configured min/max fee bounds if set
uint256 minFee = bridgeFees.minFee;
uint256 maxFee = bridgeFees.maxFee;
if (minFee > 0 && fee < minFee) {
fee = minFee;
}
if (maxFee > 0 && fee > maxFee) {
fee = maxFee;
}
// Ensure the fee does not consume the entire amount (and avoid underflow)
require(fee < _amountLD, "GoodDollarOFTAdapter: fee exceeds amount");
}
// Mint tokens to recipient (amount minus fee)
uint256 amountToRecipient = _amountLD - fee;
minterBurner.mint(_to, amountToRecipient);
```
These edits assume:
1. `bridgeFees` is a struct with `minFee` and `maxFee` fields of type `uint256`. If the types or names differ, adjust the local `minFee`/`maxFee` variables accordingly.
2. No custom error type is currently defined for the "fee exceeds amount" case. If your codebase prefers custom errors, you may want to:
- Declare something like `error FeeExceedsAmount();` at the top of the contract.
- Replace the `require` with `if (fee >= _amountLD) revert FeeExceedsAmount();`.
</issue_to_address>
### Comment 3
<location> `packages/bridge-contracts/scripts/oft/oft-deploy.ts:108-113` </location>
<code_context>
+ if (!release.GoodDollarOFTAdapter) {
+ console.log("Deploying GoodDollarOFTAdapter (non-upgradeable)...");
+
+ OFTAdapter = (await deployDeterministic(
+ {
+ name: "GoodDollarOFTAdapter",
+ isUpgradeable: false
+ },
+ [tokenAddress, MinterBurner.address, lzEndpoint, owner]
+ ).then(printDeploy)) as Contract;
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Arguments passed to `GoodDollarOFTAdapter` constructor don’t match the Solidity signature and `initialize` is never called.
Here you deploy the non-upgradeable contract with `[tokenAddress, MinterBurner.address, lzEndpoint, owner]`, but the contract’s constructor only accepts `(address _token, address _lzEndpoint)`. The minter/owner arguments should be passed via `initialize`, not the constructor. For a non-upgradeable deployment, you should (1) call the constructor with only `tokenAddress` and `lzEndpoint`, then (2) call `initialize(tokenAddress, MinterBurner.address, lzEndpoint, avatar, feeRecipient, nameService)` explicitly, or otherwise switch to an upgradeable/proxy pattern and rely solely on the initializer.
</issue_to_address>
### Comment 4
<location> `packages/bridge-contracts/scripts/oft/oft-deploy.ts:58-59` </location>
<code_context>
+ }
+ const NameService = await ethers.getContractAt("NameService", nameServiceAddress);
+
+ // Get owner - use Avatar if available, otherwise use deployer
+ const owner = root.address;
+
+ console.log("settings:", settings);
</code_context>
<issue_to_address>
**issue (bug_risk):** Comment says owner should be Avatar, but `owner` is set to the deployer, which can be misleading and affect governance assumptions.
`owner` is still initialized as `root.address` and passed into the deployment, but later you fetch `avatarAddress` and log “Owner set to DAO avatar”, even though that address is never used as the owner. Please either pass the Avatar address as `owner`, or update the comment/logging so it clearly reflects that the deployer is the effective owner.
</issue_to_address>
### Comment 5
<location> `packages/bridge-contracts/scripts/oft/set-minter-burner-limits.ts:65-68` </location>
<code_context>
+ console.log("Weekly Burn Limit:", ethers.utils.formatEther(currentWeeklyBurnLimit), "G$");
+ console.log("Monthly Burn Limit:", ethers.utils.formatEther(currentMonthlyBurnLimit), "G$");
+
+ const weeklyMintLimit = Number(process.env.WEEKLY_MINT_LIMIT);
+ const monthlyMintLimit = Number(process.env.MONTHLY_MINT_LIMIT);
+ const weeklyBurnLimit = Number(process.env.WEEKLY_BURN_LIMIT);
+ const monthlyBurnLimit = Number(process.env.MONTHLY_BURN_LIMIT);
+
+ // Check if any limits are being set
</code_context>
<issue_to_address>
**issue (bug_risk):** Environment limits are parsed as JS numbers without 18‑decimals scaling, and NaN handling/BigNumber comparisons are incorrect.
Two issues:
1) Units: Env vars are parsed as plain numbers, but limits are documented as 18‑decimals G$. `Number(process.env.X)` gives an unscaled integer (e.g. `1000000`), which you then pass to `formatEther`/ABI. If the env value is in “tokens”, you should parse the string with `ethers.utils.parseEther` (or equivalent) to convert to wei before encoding.
2) Validation/comparisons: When an env var is unset, `Number(undefined)` is `NaN`, which your `== undefined` checks won’t catch. That lets `NaN` flow into calldata and makes `limit != currentWeeklyMintLimit` (number vs BigNumber) always true. Instead, validate the raw string (`process.env.X`) for `undefined`, then convert to a `BigNumber` and compare with `BigNumber.eq`.
</issue_to_address>
### Comment 6
<location> `packages/bridge-contracts/scripts/oft/bridge-oft-token.ts:144-153` </location>
<code_context>
+ console.log(`\nRecipient on ${destNetwork}:`, recipient);
+
+ // Get destination network OFT adapter address
+ let destNetworkName: string;
+ if (isXDC) {
+ // Bridging to CELO - try production-celo first, then development-celo
+ destNetworkName = "development-celo";
+ } else {
+ // Bridging to XDC - try production-xdc first, then development-xdc
+ destNetworkName = "development-xdc";
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Destination network selection ignores production networks and is hardcoded to development variants.
`destNetworkName` is always set to a development network, even when the source is production. For example, a run on `production-xdc` still resolves deployments under `development-celo`, which is unexpected and likely to cause mistakes. It’d be safer to derive the destination env from the source (e.g., `production-xdc` → `production-celo`, `development-xdc` → `development-celo`) or allow it to be explicitly configured via CLI/env vars.
```suggestion
// Get destination network OFT adapter address
// Optionally allow explicit override via env var (e.g. "production-celo", "development-xdc")
const destNetworkOverride = process.env.DEST_NETWORK_NAME;
let destNetworkName: string;
if (destNetworkOverride && destNetworkOverride.trim().length > 0) {
destNetworkName = destNetworkOverride;
} else {
// Derive destination environment from the current Hardhat network
// e.g. "production-xdc" -> "production-celo", "development-xdc" -> "development-celo"
const sourceNetworkName = network.name;
const [envPrefix] = sourceNetworkName.split("-"); // "production" | "development" | ...
const normalizedEnv =
envPrefix === "production" || envPrefix === "development"
? envPrefix
: "development";
if (isXDC) {
// Bridging from XDC to CELO
destNetworkName = `${normalizedEnv}-celo`;
} else {
// Bridging from CELO to XDC
destNetworkName = `${normalizedEnv}-xdc`;
}
}
```
</issue_to_address>
### Comment 7
<location> `packages/bridge-contracts/scripts/oft/verify-oft-contracts.sh:24-33` </location>
<code_context>
+ # GoodDollarMinterBurner
+ # Constructor: (token, owner)
+ echo "Verifying GoodDollarMinterBurner..."
+ yarn hardhat verify \
+ --network $NETWORK \
+ --contract contracts/token/oft/GoodDollarMinterBurner.sol:GoodDollarMinterBurner \
+ 0x57D60087b69b7bA13afbAbc41CB56A24A8B648F8 \
+ 0xFa51eFDc0910CCdA91732e6806912Fa12e2FD475 \
+ 0x14204392270CD38D02C43465909Cade33BF5D6E5
+
+ echo ""
+ echo "Verifying GoodDollarOFTAdapter..."
+ # GoodDollarOFTAdapter
+ # Constructor: (token, minterBurner, lzEndpoint, owner)
+ yarn hardhat verify \
+ --network $NETWORK \
+ --contract contracts/token/oft/GoodDollarOFTAdapter.sol:GoodDollarOFTAdapter \
+ 0xb8e2aae105fb0637B08806e855057A0985bf859c \
+ 0xFa51eFDc0910CCdA91732e6806912Fa12e2FD475 \
</code_context>
<issue_to_address>
**issue (bug_risk):** Verification script paths and constructor parameters don’t align with the actual contracts and their signatures.
The verify commands target `contracts/token/oft/GoodDollarMinterBurner.sol` and assume `(token, owner)` (plus extra args for the adapter), but the actual contracts are under `packages/bridge-contracts/contracts/oft/` and use `GoodDollarMinterBurner.initialize(ISuperGoodDollar, INameService)` and `GoodDollarOFTAdapter(address _token, address _lzEndpoint)`. Please update the file paths to the correct directory and adjust the argument lists to match the real constructor/initialization flow so the verification runs against the right artifacts.
</issue_to_address>
### Comment 8
<location> `packages/bridge-contracts/scripts/oft/bridge-oft-token.ts:46` </location>
<code_context>
+ const [sender] = await ethers.getSigners();
+
+ // Detect source and destination networks
+ const isXDC = networkName.includes("xdc");
+ const isCELO = networkName.includes("celo");
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the network/endpoint derivation and repeated allowance/approval logic into small helpers to simplify `main` and reduce duplication without changing behavior.
You can reduce complexity and duplication without changing behavior by extracting a couple of small helpers. For example:
1. Centralize network / endpoint / naming logic
Right now `isXDC`, `isCELO`, `sourceNetwork`, `destNetwork`, `sourceEndpointId`, `destEndpointId`, `nativeTokenName`, and `destNetworkName` are derived inline. That can move to a single helper:
```ts
type BridgeContext = {
sourceNetwork: "XDC" | "CELO";
destNetwork: "XDC" | "CELO";
sourceEndpointId: number;
destEndpointId: number;
nativeTokenName: "XDC" | "CELO";
destNetworkName: string;
};
function getBridgeContext(networkName: string): BridgeContext {
const isXDC = networkName.includes("xdc");
const isCELO = networkName.includes("celo");
if (!isXDC && !isCELO) {
throw new Error(
`Network must be XDC or CELO. Current network: ${networkName}\n` +
`Supported networks: production-xdc, development-xdc, production-celo, development-celo`
);
}
const sourceNetwork = isXDC ? "XDC" : "CELO";
const destNetwork = isXDC ? "CELO" : "XDC";
const sourceEndpointId = isXDC ? XDC_ENDPOINT_ID : CELO_ENDPOINT_ID;
const destEndpointId = isXDC ? CELO_ENDPOINT_ID : XDC_ENDPOINT_ID;
const nativeTokenName = isXDC ? "XDC" : "CELO";
const destNetworkName = isXDC ? "development-celo" : "development-xdc";
return {
sourceNetwork,
destNetwork,
sourceEndpointId,
destEndpointId,
nativeTokenName,
destNetworkName,
};
}
```
Usage in `main` becomes simpler:
```ts
const networkName = network.name;
const [sender] = await ethers.getSigners();
const {
sourceNetwork,
destNetwork,
sourceEndpointId,
destEndpointId,
nativeTokenName,
destNetworkName,
} = getBridgeContext(networkName);
```
2. Deduplicate allowance / approval flow
The MinterBurner and OFT adapter allowance logic is almost identical. A tiny helper keeps the logs and behavior but removes duplication:
```ts
async function ensureAllowance(
token: Contract,
owner: string,
spender: string,
amount: ethers.BigNumber,
label: string
) {
const current = await token.allowance(owner, spender);
console.log(`\nChecking ${label} allowance...`);
console.log(`Current ${label} allowance:`, ethers.utils.formatEther(current), "G$");
if (current.lt(amount)) {
console.log(`\nApproving ${label} to spend tokens...`);
const tx = await token.approve(spender, amount);
await tx.wait();
console.log(`${label} approval confirmed:`, tx.hash);
} else {
console.log(`Sufficient ${label} allowance already set`);
}
}
```
Then in `main`:
```ts
await ensureAllowance(token, sender.address, minterBurnerAddress, amount, "MinterBurner");
await ensureAllowance(token, sender.address, oftAdapterAddress, amount, "OFT adapter");
```
The later “final allowance” check can stay as-is or be wrapped similarly in a smaller helper (e.g. `assertMinAllowance`), but even just extracting `ensureAllowance` meaningfully cuts repetition and improves readability without altering functionality.
</issue_to_address>
### Comment 9
<location> `packages/bridge-contracts/scripts/oft/oft-deploy.ts:30` </location>
<code_context>
+
+const { name: networkName } = network;
+
+export const deployOFTContracts = async () => {
+ const isProduction = networkName.includes("production");
+ let [root] = await ethers.getSigners();
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring this deployment script into smaller reusable helpers so that deployOFTContracts becomes a clearer high-level orchestration function.
You can reduce the complexity of this script without changing behavior by extracting a few small helpers and reusing patterns you already use elsewhere.
### 1. Extract deployment context
Pulling network-related context into a helper shrinks `deployOFTContracts` and clarifies responsibilities:
```ts
type DeploymentContext = {
networkName: string;
root: string;
release: { [key: string]: any };
settings: any;
tokenAddress: string;
nameServiceAddress: string;
lzEndpoint: string;
};
async function getDeploymentContext(): Promise<DeploymentContext> {
const { name: networkName } = network;
const isProduction = networkName.includes("production");
const [rootSigner] = await ethers.getSigners();
if (isProduction) verifyProductionSigner(rootSigner);
const release = dao[networkName];
const settings = ProtocolSettings[networkName];
const tokenAddress = release.GoodDollar;
if (!tokenAddress) {
throw new Error(
`Token address not found in deployment.json for network ${networkName}. Please deploy SuperGoodDollar or GoodDollar first.`
);
}
const nameServiceAddress = release.NameService;
if (!nameServiceAddress) {
throw new Error(
`NameService address not found in deployment.json for network ${networkName}. Please deploy NameService first.`
);
}
const lzEndpoint = settings.layerZero?.endpoint;
if (!lzEndpoint) {
throw new Error(
`LayerZero endpoint not found. Please set it in deploy-settings.json under layerZero.endpoint or set LAYERZERO_ENDPOINT environment variable.`
);
}
return {
networkName,
root: rootSigner.address,
release,
settings,
tokenAddress,
nameServiceAddress,
lzEndpoint
};
}
```
Then `deployOFTContracts` starts with:
```ts
export const deployOFTContracts = async () => {
const ctx = await getDeploymentContext();
const NameService = await ethers.getContractAt("NameService", ctx.nameServiceAddress);
const Controller = await ethers.getContractAt("Controller", await NameService.getAddress("CONTROLLER"));
const avatarAddress = await Controller.avatar();
// ...
};
```
### 2. Abstract the deploy-or-get pattern
You repeat the same “deploy deterministic or load existing” logic. A small helper keeps behavior identical and reduces duplication:
```ts
async function deployOrGetDeterministic(
releaseKey: keyof typeof dao[string],
deployConfig: { name: string; isUpgradeable: boolean; initializer?: string },
args: any[],
networkName: string
): Promise<Contract> {
const release = dao[networkName];
if (!release[releaseKey]) {
console.log(`Deploying ${deployConfig.name}...`);
const instance = (await deployDeterministic(deployConfig, args).then(printDeploy)) as Contract;
await releaser({ [releaseKey]: instance.address }, networkName, "deployment", false);
console.log(`${deployConfig.name} deployed to:`, instance.address);
return instance;
}
console.log(`${deployConfig.name} already deployed at:`, release[releaseKey]);
return ethers.getContractAt(deployConfig.name, release[releaseKey]);
}
```
Usage:
```ts
const MinterBurner = await deployOrGetDeterministic(
"GoodDollarMinterBurner",
{ name: "GoodDollarMinterBurner", isUpgradeable: true, initializer: "initialize" },
[ctx.tokenAddress, ctx.nameServiceAddress],
ctx.networkName
);
const OFTAdapter = await deployOrGetDeterministic(
"GoodDollarOFTAdapter",
{ name: "GoodDollarOFTAdapter", isUpgradeable: false },
[ctx.tokenAddress, MinterBurner.address, ctx.lzEndpoint, ctx.root],
ctx.networkName
);
```
### 3. Extract DAO operator wiring
The operator setup can be a focused helper; you already isolate similar governance flows elsewhere:
```ts
async function ensureOFTAdapterIsOperator(
MinterBurner: Contract,
OFTAdapter: Contract,
Controller: Contract,
avatarAddress: string
): Promise<void> {
const isOperator = await MinterBurner.operators(OFTAdapter.address);
if (isOperator) {
console.log("OFT adapter is already an operator on MinterBurner");
return;
}
console.log("Setting OFT adapter as operator on MinterBurner via DAO...");
console.log(` MinterBurner address: ${MinterBurner.address}`);
console.log(` OFTAdapter address: ${OFTAdapter.address}`);
const data = MinterBurner.interface.encodeFunctionData("setOperator", [
OFTAdapter.address,
true
]);
const tx = await Controller.genericCall(MinterBurner.address, data, avatarAddress, 0);
await tx.wait();
console.log("✅ Successfully set OFT adapter as operator on MinterBurner");
console.log("Transaction hash:", tx.hash);
const isOperatorAfter = await MinterBurner.operators(OFTAdapter.address);
if (!isOperatorAfter) {
console.log("⚠️ Warning: Operator status not set. Please check the transaction.");
} else {
console.log("✅ Verified: OFT adapter is now an operator");
}
}
```
Then in `deployOFTContracts`:
```ts
await ensureOFTAdapterIsOperator(MinterBurner, OFTAdapter, Controller, avatarAddress);
```
If you already have an `executeViaGuardian` or similar governance helper, you can delegate the genericCall part to it inside `ensureOFTAdapterIsOperator` to align with other scripts.
### 4. Remove unused imports
`defaultsDeep` and `Signer` aren’t used:
```ts
- import { Contract, Signer } from "ethers";
- import { defaultsDeep } from "lodash";
+ import { Contract } from "ethers";
```
These changes keep the behavior and flow intact, but make `deployOFTContracts` essentially a high-level orchestration function that’s easier to read and maintain.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol
Outdated
Show resolved
Hide resolved
packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol
Outdated
Show resolved
Hide resolved
Comment on lines
58
to
59
| // Get owner - use Avatar if available, otherwise use deployer | ||
| const owner = root.address; |
There was a problem hiding this comment.
issue (bug_risk): Comment says owner should be Avatar, but owner is set to the deployer, which can be misleading and affect governance assumptions.
owner is still initialized as root.address and passed into the deployment, but later you fetch avatarAddress and log “Owner set to DAO avatar”, even though that address is never used as the owner. Please either pass the Avatar address as owner, or update the comment/logging so it clearly reflects that the deployer is the effective owner.
Comment on lines
65
to
68
| const weeklyMintLimit = Number(process.env.WEEKLY_MINT_LIMIT); | ||
| const monthlyMintLimit = Number(process.env.MONTHLY_MINT_LIMIT); | ||
| const weeklyBurnLimit = Number(process.env.WEEKLY_BURN_LIMIT); | ||
| const monthlyBurnLimit = Number(process.env.MONTHLY_BURN_LIMIT); |
There was a problem hiding this comment.
issue (bug_risk): Environment limits are parsed as JS numbers without 18‑decimals scaling, and NaN handling/BigNumber comparisons are incorrect.
Two issues:
- Units: Env vars are parsed as plain numbers, but limits are documented as 18‑decimals G$.
Number(process.env.X)gives an unscaled integer (e.g.1000000), which you then pass toformatEther/ABI. If the env value is in “tokens”, you should parse the string withethers.utils.parseEther(or equivalent) to convert to wei before encoding. - Validation/comparisons: When an env var is unset,
Number(undefined)isNaN, which your== undefinedchecks won’t catch. That letsNaNflow into calldata and makeslimit != currentWeeklyMintLimit(number vs BigNumber) always true. Instead, validate the raw string (process.env.X) forundefined, then convert to aBigNumberand compare withBigNumber.eq.
| const [sender] = await ethers.getSigners(); | ||
|
|
||
| // Detect source and destination networks | ||
| const isXDC = networkName.includes("xdc"); |
There was a problem hiding this comment.
issue (complexity): Consider extracting the network/endpoint derivation and repeated allowance/approval logic into small helpers to simplify main and reduce duplication without changing behavior.
You can reduce complexity and duplication without changing behavior by extracting a couple of small helpers. For example:
- Centralize network / endpoint / naming logic
Right now isXDC, isCELO, sourceNetwork, destNetwork, sourceEndpointId, destEndpointId, nativeTokenName, and destNetworkName are derived inline. That can move to a single helper:
type BridgeContext = {
sourceNetwork: "XDC" | "CELO";
destNetwork: "XDC" | "CELO";
sourceEndpointId: number;
destEndpointId: number;
nativeTokenName: "XDC" | "CELO";
destNetworkName: string;
};
function getBridgeContext(networkName: string): BridgeContext {
const isXDC = networkName.includes("xdc");
const isCELO = networkName.includes("celo");
if (!isXDC && !isCELO) {
throw new Error(
`Network must be XDC or CELO. Current network: ${networkName}\n` +
`Supported networks: production-xdc, development-xdc, production-celo, development-celo`
);
}
const sourceNetwork = isXDC ? "XDC" : "CELO";
const destNetwork = isXDC ? "CELO" : "XDC";
const sourceEndpointId = isXDC ? XDC_ENDPOINT_ID : CELO_ENDPOINT_ID;
const destEndpointId = isXDC ? CELO_ENDPOINT_ID : XDC_ENDPOINT_ID;
const nativeTokenName = isXDC ? "XDC" : "CELO";
const destNetworkName = isXDC ? "development-celo" : "development-xdc";
return {
sourceNetwork,
destNetwork,
sourceEndpointId,
destEndpointId,
nativeTokenName,
destNetworkName,
};
}Usage in main becomes simpler:
const networkName = network.name;
const [sender] = await ethers.getSigners();
const {
sourceNetwork,
destNetwork,
sourceEndpointId,
destEndpointId,
nativeTokenName,
destNetworkName,
} = getBridgeContext(networkName);- Deduplicate allowance / approval flow
The MinterBurner and OFT adapter allowance logic is almost identical. A tiny helper keeps the logs and behavior but removes duplication:
async function ensureAllowance(
token: Contract,
owner: string,
spender: string,
amount: ethers.BigNumber,
label: string
) {
const current = await token.allowance(owner, spender);
console.log(`\nChecking ${label} allowance...`);
console.log(`Current ${label} allowance:`, ethers.utils.formatEther(current), "G$");
if (current.lt(amount)) {
console.log(`\nApproving ${label} to spend tokens...`);
const tx = await token.approve(spender, amount);
await tx.wait();
console.log(`${label} approval confirmed:`, tx.hash);
} else {
console.log(`Sufficient ${label} allowance already set`);
}
}Then in main:
await ensureAllowance(token, sender.address, minterBurnerAddress, amount, "MinterBurner");
await ensureAllowance(token, sender.address, oftAdapterAddress, amount, "OFT adapter");The later “final allowance” check can stay as-is or be wrapped similarly in a smaller helper (e.g. assertMinAllowance), but even just extracting ensureAllowance meaningfully cuts repetition and improves readability without altering functionality.
|
|
||
| const { name: networkName } = network; | ||
|
|
||
| export const deployOFTContracts = async () => { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring this deployment script into smaller reusable helpers so that deployOFTContracts becomes a clearer high-level orchestration function.
You can reduce the complexity of this script without changing behavior by extracting a few small helpers and reusing patterns you already use elsewhere.
1. Extract deployment context
Pulling network-related context into a helper shrinks deployOFTContracts and clarifies responsibilities:
type DeploymentContext = {
networkName: string;
root: string;
release: { [key: string]: any };
settings: any;
tokenAddress: string;
nameServiceAddress: string;
lzEndpoint: string;
};
async function getDeploymentContext(): Promise<DeploymentContext> {
const { name: networkName } = network;
const isProduction = networkName.includes("production");
const [rootSigner] = await ethers.getSigners();
if (isProduction) verifyProductionSigner(rootSigner);
const release = dao[networkName];
const settings = ProtocolSettings[networkName];
const tokenAddress = release.GoodDollar;
if (!tokenAddress) {
throw new Error(
`Token address not found in deployment.json for network ${networkName}. Please deploy SuperGoodDollar or GoodDollar first.`
);
}
const nameServiceAddress = release.NameService;
if (!nameServiceAddress) {
throw new Error(
`NameService address not found in deployment.json for network ${networkName}. Please deploy NameService first.`
);
}
const lzEndpoint = settings.layerZero?.endpoint;
if (!lzEndpoint) {
throw new Error(
`LayerZero endpoint not found. Please set it in deploy-settings.json under layerZero.endpoint or set LAYERZERO_ENDPOINT environment variable.`
);
}
return {
networkName,
root: rootSigner.address,
release,
settings,
tokenAddress,
nameServiceAddress,
lzEndpoint
};
}Then deployOFTContracts starts with:
export const deployOFTContracts = async () => {
const ctx = await getDeploymentContext();
const NameService = await ethers.getContractAt("NameService", ctx.nameServiceAddress);
const Controller = await ethers.getContractAt("Controller", await NameService.getAddress("CONTROLLER"));
const avatarAddress = await Controller.avatar();
// ...
};2. Abstract the deploy-or-get pattern
You repeat the same “deploy deterministic or load existing” logic. A small helper keeps behavior identical and reduces duplication:
async function deployOrGetDeterministic(
releaseKey: keyof typeof dao[string],
deployConfig: { name: string; isUpgradeable: boolean; initializer?: string },
args: any[],
networkName: string
): Promise<Contract> {
const release = dao[networkName];
if (!release[releaseKey]) {
console.log(`Deploying ${deployConfig.name}...`);
const instance = (await deployDeterministic(deployConfig, args).then(printDeploy)) as Contract;
await releaser({ [releaseKey]: instance.address }, networkName, "deployment", false);
console.log(`${deployConfig.name} deployed to:`, instance.address);
return instance;
}
console.log(`${deployConfig.name} already deployed at:`, release[releaseKey]);
return ethers.getContractAt(deployConfig.name, release[releaseKey]);
}Usage:
const MinterBurner = await deployOrGetDeterministic(
"GoodDollarMinterBurner",
{ name: "GoodDollarMinterBurner", isUpgradeable: true, initializer: "initialize" },
[ctx.tokenAddress, ctx.nameServiceAddress],
ctx.networkName
);
const OFTAdapter = await deployOrGetDeterministic(
"GoodDollarOFTAdapter",
{ name: "GoodDollarOFTAdapter", isUpgradeable: false },
[ctx.tokenAddress, MinterBurner.address, ctx.lzEndpoint, ctx.root],
ctx.networkName
);3. Extract DAO operator wiring
The operator setup can be a focused helper; you already isolate similar governance flows elsewhere:
async function ensureOFTAdapterIsOperator(
MinterBurner: Contract,
OFTAdapter: Contract,
Controller: Contract,
avatarAddress: string
): Promise<void> {
const isOperator = await MinterBurner.operators(OFTAdapter.address);
if (isOperator) {
console.log("OFT adapter is already an operator on MinterBurner");
return;
}
console.log("Setting OFT adapter as operator on MinterBurner via DAO...");
console.log(` MinterBurner address: ${MinterBurner.address}`);
console.log(` OFTAdapter address: ${OFTAdapter.address}`);
const data = MinterBurner.interface.encodeFunctionData("setOperator", [
OFTAdapter.address,
true
]);
const tx = await Controller.genericCall(MinterBurner.address, data, avatarAddress, 0);
await tx.wait();
console.log("✅ Successfully set OFT adapter as operator on MinterBurner");
console.log("Transaction hash:", tx.hash);
const isOperatorAfter = await MinterBurner.operators(OFTAdapter.address);
if (!isOperatorAfter) {
console.log("⚠️ Warning: Operator status not set. Please check the transaction.");
} else {
console.log("✅ Verified: OFT adapter is now an operator");
}
}Then in deployOFTContracts:
await ensureOFTAdapterIsOperator(MinterBurner, OFTAdapter, Controller, avatarAddress);If you already have an executeViaGuardian or similar governance helper, you can delegate the genericCall part to it inside ensureOFTAdapterIsOperator to align with other scripts.
4. Remove unused imports
defaultsDeep and Signer aren’t used:
- import { Contract, Signer } from "ethers";
- import { defaultsDeep } from "lodash";
+ import { Contract } from "ethers";These changes keep the behavior and flow intact, but make deployOFTContracts essentially a high-level orchestration function that’s easier to read and maintain.
Feature: Bridge GoodDollar using OFT adapter via LayerZero Endpoint v2
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
…ts, and related logic to enhance clarity and reduce complexity
…cluding Hardhat version upgrades and new remapping file
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
This PR implements cross-chain bridging for GoodDollar (G$) tokens using LayerZero's Omnichain Fungible Token (OFT) v2 adapter. The implementation enables seamless token transfers between XDC and CELO networks.
About #7
How Has This Been Tested?
Bridge functionality should be tested on XDC and CELO.
Checklist: