Chore: clean up OFT deploy/configure scripts#12
Merged
Conversation
… endpoint retrieval and updating configuration references in documentation.
![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 6 issues, and left some high level feedback:
- The
parseLimitheuristic inscripts/oft/steps/set-bridge-limits.ts(length check + decimal detection) is a bit opaque; consider making the units explicit in config (e.g., always G$ as a decimal string) and removing the BigNumber fallback or at least documenting this behavior directly in code. - In
configure-oft.ts,resolveOftPairNetworksinfers production vs development purely fromnetworkName.includes("production"); if you expect additional environments or differently named networks, it may be safer to use an explicit mapping or a stricter pattern to avoid misrouting wiring. - The removal of the detailed error diagnostics in
scripts/oft/bridge-oft-token.tsreduces runtime guidance for common LayerZero misconfigurations; consider keeping a lightweight version of those checks or moving them into a reusable helper so operators still get actionable hints when bridging fails.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `parseLimit` heuristic in `scripts/oft/steps/set-bridge-limits.ts` (length check + decimal detection) is a bit opaque; consider making the units explicit in config (e.g., always G$ as a decimal string) and removing the BigNumber fallback or at least documenting this behavior directly in code.
- In `configure-oft.ts`, `resolveOftPairNetworks` infers production vs development purely from `networkName.includes("production")`; if you expect additional environments or differently named networks, it may be safer to use an explicit mapping or a stricter pattern to avoid misrouting wiring.
- The removal of the detailed error diagnostics in `scripts/oft/bridge-oft-token.ts` reduces runtime guidance for common LayerZero misconfigurations; consider keeping a lightweight version of those checks or moving them into a reusable helper so operators still get actionable hints when bridging fails.
## Individual Comments
### Comment 1
<location path="packages/bridge-contracts/deploy/utils/getLzEndpoint.ts" line_range="11-15" />
<code_context>
+ "production-xdc": xdcMainnetEndpoint.address,
+};
+
+export function getLzEndpoint(networkName: string): string {
+ const fromEnv = process.env.LAYERZERO_ENDPOINT;
+ if (fromEnv) return fromEnv;
+
+ const endpoint = LZ_ENDPOINT_BY_NETWORK[networkName];
+ if (!endpoint) {
+ throw new Error(
</code_context>
<issue_to_address>
**question (bug_risk):** Environment variable now overrides network mapping; consider whether this behavioral change is intended.
The previous logic only used `process.env.LAYERZERO_ENDPOINT` when no per-network mapping existed; now it always overrides mapped addresses. This increases flexibility but also makes a misconfigured env var silently override correct network-specific endpoints. If the old precedence was intentional, consider only using the env var as a fallback or add validation/logging when it’s present.
</issue_to_address>
### Comment 2
<location path="packages/bridge-contracts/scripts/oft/run-configure.js" line_range="24-33" />
<code_context>
+
+function wireLayerZero(networkName: string) {
+ const pair = resolveOftPairNetworks(networkName);
+ const result = spawnSync(
+ "npx",
+ ["hardhat", "lz:oapp:wire", "--oapp-config", "./scripts/oft/layerzero.config.ts", "--network", networkName, "--ci"],
</code_context>
<issue_to_address>
**suggestion:** Child process errors are not surfaced when `spawnSync` fails without a status code.
When `spawnSync` fails (e.g., `npx`/`hardhat` missing), `result.status` may be `null` and the error only appears in `result.error`. Right now we exit with code 1 but don’t log that error, which obscures the root cause in CI. Please check `result.error` and log it before exiting so these failures are easier to diagnose.
</issue_to_address>
### Comment 3
<location path="packages/bridge-contracts/scripts/oft/OFT_CONFIGURING_GUIDE.md" line_range="15" />
<code_context>
+npx hardhat deploy --tags OFT --network production-celo
+```
+
+Use `development-xdc` / `development-celo` for staging. Deploy **both** chains before configure (wiring needs both adapters).
+
+### Deploy parameters (`deploy/deployOFT.ts`)
</code_context>
<issue_to_address>
**suggestion (typo):** Consider changing "before configure" to "before configuring" or "before configuration" for smoother grammar.
This phrasing is slightly ungrammatical; using "before configuring" or "before configuration" would read more naturally.
```suggestion
Use `development-xdc` / `development-celo` for staging. Deploy **both** chains before configuring (wiring needs both adapters).
```
</issue_to_address>
### Comment 4
<location path="packages/bridge-contracts/scripts/oft/configure-oft.ts" line_range="29" />
<code_context>
+ };
+}
+
+function resolveOftPairNetworks(networkName: string): { xdc: string; celo: string } {
+ const isProduction = networkName.includes("production");
+ return {
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the configuration script to use explicit network mappings, a dedicated wiring runner, and an injectable flag source to simplify orchestration and make behavior clearer.
You can reduce the orchestration complexity without changing behavior by making the configuration more explicit and isolating responsibilities.
### 1. Make network pairing explicit instead of relying on naming heuristics
`resolveOftPairNetworks` currently couples behavior to `networkName.includes("production")`. A small explicit map keeps the behavior the same but makes it clearer and safer:
```ts
// Instead of inferring via "includes"
function resolveOftPairNetworks(networkName: string): { xdc: string; celo: string } {
const pairs: Record<string, { xdc: string; celo: string }> = {
"production-xdc": { xdc: "production-xdc", celo: "production-celo" },
"production-celo": { xdc: "production-xdc", celo: "production-celo" },
"development-xdc": { xdc: "development-xdc", celo: "development-celo" },
"development-celo": { xdc: "development-xdc", celo: "development-celo" },
// add any other aliases explicitly
};
const pair = pairs[networkName];
if (!pair) {
throw new Error(`Unknown network pairing for "${networkName}"`);
}
return pair;
}
```
This keeps the same production/development pairing, but removes the hidden dependency on string naming conventions.
### 2. Separate wiring invocation from env/network resolution
`wireLayerZero` currently mixes pair resolution, env building, and process spawning. Extract the spawn call into a small utility that takes explicit parameters:
```ts
function runLayerZeroWire(networkName: string, xdcNetwork: string, celoNetwork: string) {
const result = spawnSync(
"npx",
[
"hardhat",
"lz:oapp:wire",
"--oapp-config",
"./scripts/oft/layerzero.config.ts",
"--network",
networkName,
"--ci",
],
{
cwd: path.resolve(__dirname, "../.."),
stdio: "inherit",
env: {
...process.env,
OFT_XDC_NETWORK: xdcNetwork,
OFT_CELO_NETWORK: celoNetwork,
},
}
);
if (result.error) throw result.error;
if (result.status !== 0) {
throw new Error(`LayerZero wire failed with exit code ${result.status ?? "unknown"}`);
}
}
function wireLayerZero(networkName: string) {
const pair = resolveOftPairNetworks(networkName);
runLayerZeroWire(networkName, pair.xdc, pair.celo);
}
```
This keeps all behavior intact while making the wiring path easier to understand and test (you can unit‑test `resolveOftPairNetworks` separately from `runLayerZeroWire`).
### 3. Decouple flag parsing from env to simplify orchestration
You can keep env support but move it behind an explicit flag source, making it easier to later add `argv` parsing or a Hardhat task without touching the orchestration logic:
```ts
type ConfigureFlags = {
skipMinter: boolean;
skipLimits: boolean;
skipWire: boolean;
skipAdapterOwnership: boolean;
};
function flagsFromEnv(): ConfigureFlags {
const flagEnabled = (name: string): boolean => {
const value = process.env[name];
return value === "1" || value === "true";
};
return {
skipMinter: flagEnabled("SKIP_MINTER"),
skipLimits: flagEnabled("SKIP_LIMITS"),
skipWire: flagEnabled("SKIP_WIRE"),
skipAdapterOwnership: flagEnabled("SKIP_ADAPTER_OWNERSHIP"),
};
}
// main orchestration only cares about a ConfigureFlags object
export const main = async (flags: ConfigureFlags = flagsFromEnv()) => {
const networkName = network.name;
console.log(`oft:configure ${networkName}`, flags);
if (!flags.skipMinter) await grantMinterRole();
if (!flags.skipLimits) await setBridgeLimits();
if (!flags.skipWire) wireLayerZero(networkName);
if (!flags.skipAdapterOwnership) await transferAdapterOwnership();
console.log(`oft:configure done (${networkName})`);
};
```
With this, `main` is purely driven by a `ConfigureFlags` object. A future `argv`-based wrapper or Hardhat task can construct those flags directly, reducing the need for env‑only indirection while preserving current env behavior.
</issue_to_address>
### Comment 5
<location path="packages/bridge-contracts/scripts/oft/run-configure.js" line_range="1" />
<code_context>
+const { spawnSync } = require("child_process");
+const path = require("path");
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the wrapper script by passing CLI flags directly through to `configure-oft.ts` and handling all flag parsing there instead of via environment variables.
You can keep the same functionality (CLI flags to skip steps) while removing the indirection via env variables and the custom flag mapping. Let `configure-oft.ts` own its flag parsing and make this wrapper a thin pass-through.
Wrapper simplification:
```js
const { spawnSync } = require("child_process");
const path = require("path");
const hardhatArgs = process.argv.slice(2);
const result = spawnSync(
"npx",
["hardhat", "run", "scripts/oft/configure-oft.ts", ...hardhatArgs],
{
cwd: path.resolve(__dirname, "../.."),
stdio: "inherit",
env: process.env,
}
);
process.exit(result.status ?? 1);
```
Then in `scripts/oft/configure-oft.ts`, read the flags directly from `process.argv` instead of env:
```ts
const args = process.argv.slice(2);
const skipMinter = args.includes("--skip-minter");
const skipLimits = args.includes("--skip-limits");
const skipWire = args.includes("--skip-wire");
const skipAdapterOwnership = args.includes("--skip-adapter-ownership");
// use these booleans in the existing flow
```
This keeps the current behavior (same flags, same steps being skipped) but removes the duplicated flag handling and the hidden coupling via environment variable names, making the configuration flow easier to understand and maintain.
</issue_to_address>
### Comment 6
<location path="packages/bridge-contracts/scripts/oft/steps/set-bridge-limits.ts" line_range="6" />
<code_context>
+import { getOftDeploymentAddresses } from "../../../deploy/utils/getOftDeploymentAddresses";
+import config from "../oft.config.json";
+
+function parseLimit(value: string | undefined): BigNumber | null {
+ if (!value) return null;
+ const numValue = value.trim();
</code_context>
<issue_to_address>
**issue (complexity):** Consider making the limit parsing more explicit and extracting the bridge-limits equality check into a helper to reduce cognitive load without changing behavior.
You can reduce the cognitive load without changing behavior by making the parsing and comparison logic more explicit and centralized, while still preserving the existing heuristic as a fallback.
### 1. Make `parseLimit` explicit but backwards-compatible
Allow explicit units in the config (e.g. `"1.5 ether"`, `"1000000000000000000 wei"`), and only fall back to the current heuristic when no unit is given. This keeps existing configs working but makes future configs clearer.
```ts
function parseLimit(raw: string | undefined): BigNumber | null {
if (!raw) return null;
const value = raw.trim();
// explicit unit support: "<number> ether" / "<number> wei"
const [num, unit] = value.split(/\s+/);
if (unit === "ether") {
return ethers.utils.parseEther(num);
}
if (unit === "wei") {
return ethers.BigNumber.from(num);
}
// fallback to existing heuristic for backwards compatibility
if (num.includes(".") || num.length < 15) {
return ethers.utils.parseEther(num);
}
return ethers.BigNumber.from(num);
}
```
Config examples that preserve current behavior but are now explicit:
```json
{
"mainnet": {
"limits": {
"dailyLimit": "1000 ether",
"txLimit": "5 ether",
"accountDailyLimit": "1000000000000000000 wei",
"minAmount": "0.1 ether",
"onlyWhitelisted": true
}
}
}
```
Existing configs without units (`"1000"`, `"0.1"`) will still work via the heuristic.
### 2. Factor the `unchanged` check into a small helper
Centralize the equality logic to make it easier to audit and extend:
```ts
function bridgeLimitsEqual(
a: typeof current,
b: typeof current
): boolean {
return (
a.dailyLimit.eq(b.dailyLimit) &&
a.txLimit.eq(b.txLimit) &&
a.accountDailyLimit.eq(b.accountDailyLimit) &&
a.minAmount.eq(b.minAmount) &&
a.onlyWhitelisted === b.onlyWhitelisted
);
}
// usage:
const unchanged = bridgeLimitsEqual(next, current);
```
This keeps behavior identical while making the comparison logic more discoverable and less dense in the main flow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…streamlined setup, enhancing LayerZero endpoint retrieval, and updating related documentation. Remove deprecated run-configure.js script.
…ore ownership transfer, and revise documentation for clarity on configuration steps and skip flags.
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
1 participant
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
scripts/oft/withyarn oft:configure@layerzerolabs/lz-evm-sdk-v2About # (link your issue here)
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist:
Summary by Sourcery
Centralize OFT bridge deployment and post-deploy configuration into scripts with a single oft:configure entrypoint and shared LayerZero endpoint utilities.
New Features:
Enhancements:
Build:
Documentation:
Tests:
Chores: