[VPD-817] Upgrade VBep20Delegate for core pool markets#684
Merged
Conversation
Upgrade all VBep20Delegator proxies to new implementation with internalCash tracking. Each market atomically sets the new implementation and calls sweepTokenAndSync to initialize internalCash.
- Isolated pool VToken upgrade across 7 networks: Ethereum, Arbitrum, Optimism, Base, opBNB, Unichain, ZkSync - Each file exports: - Core comptroller address (queried on-chain via getAllMarkets()) - VTokenBeacon proxy address - New VToken implementation placeholder (TODO, not yet deployed) - LayerZero destination chain ID - Core pool VToken addresses array with symbol comments - Total: 23 (ETH) + 7 (ARB) + 5 (OP) + 5 (Base) + 5 (opBNB) + 7 (Unichain) + 9 (ZkSync) = 61 Core VTokens
- Upgrade VTokenBeacon on 7 networks via upgradeTo(address) through LayerZero - Call syncCash() on each Core comptroller VToken to initialize internalCash
- ILVToken (syncCash, internalCash, accrueInterest, underlying, etc.) - ILComptroller (getAllMarkets) - ILPoolRegistry (getAllPools) - vtokenBeacon (upgradeTo, implementation)
- 7 remote network sims using testForkedNetworkVipCommands (Ethereum, Arbitrum, Optimism, Base, opBNB, Unichain, ZkSync) - BSC local sim using testVip for cross-chain dispatch verification - Post-VIP assertions: beacon implementation, internalCash == underlying balance, accrueInterest succeeds
syncCash() requires AccessControlManager authorization. Add ACM and Normal Timelock addresses to all network address files, fix EIP-55 checksums, and add giveCallPermission commands before upgrade+sync.
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces VIP-600, VIP-601, and VIP-602 governance proposals to upgrade vToken implementations and synchronize cash accounting across eight blockchain networks. It includes contract ABIs, network-specific address configurations, proposal action definitions, and comprehensive forked network test simulations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (2)
simulations/vip-608/bscmainnet.ts (2)
119-129: Assert the other accounting fields you already snapshot.
totalBorrows,totalReserves, andexchangeRateStoredare captured on Lines 123-128 but never compared after execution. AsweepTokenAndSyncregression that leaves supply unchanged would still pass.♻️ Suggested assertion expansion
- it("should preserve totalSupply", async () => { + it("should preserve core accounting fields", async () => { const snapshot = preVipSnapshots.get(marketAddress)!; expect(await vToken.totalSupply()).to.equal(snapshot.totalSupply); + expect(await vToken.totalBorrows()).to.equal(snapshot.totalBorrows); + expect(await vToken.totalReserves()).to.equal(snapshot.totalReserves); + expect(await vToken.exchangeRateStored()).to.equal(snapshot.exchangeRate); });Also applies to: 159-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-608/bscmainnet.ts` around lines 119 - 129, The test currently captures totalBorrows, totalReserves, totalSupply and exchangeRateStored into preVipSnapshots for each MARKET_ADDRESSES entry but only asserts supply later, so extend the post-VIP assertions to compare all captured fields: after the VIP run iterate MARKET_ADDRESSES and for each address retrieve the current vToken.totalBorrows(), vToken.totalReserves(), vToken.totalSupply() and vToken.exchangeRateStored() and assert equality against the values stored in preVipSnapshots (use the same keys: totalBorrows, totalReserves, totalSupply, exchangeRate) so regressions like sweepTokenAndSync that only mutate other accounting fields fail; apply the same expanded assertions to the second snapshot block referenced around the 159-162 area.
15-106: Don't keep the market set in two hardcoded arrays.This duplicates the 43-market inventory inside the test and relies on index alignment between
MARKET_ADDRESSESandMARKET_NAMES, so it can drift from the proposal payload. Prefer a shared{ name, vToken }source instead of syncing two arrays by hand.As per coding guidelines, "Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES..".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-608/bscmainnet.ts` around lines 15 - 106, Replace the two hardcoded arrays MARKET_ADDRESSES and MARKET_NAMES with a single shared data source that pairs name and vToken (e.g., an array of { name, vToken } objects) and import network addresses from src/networkAddresses.ts using NETWORK_ADDRESSES.<network>.<key> for each vToken; update any code that referenced MARKET_ADDRESSES or MARKET_NAMES to use the new combined structure (look for usages of MARKET_ADDRESSES, MARKET_NAMES, and any place that iterates by index) so the test reads names and addresses from the same authoritative source instead of relying on index alignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@simulations/vip-608/bscmainnet-2.ts`:
- Around line 3-8: The test fails because vip608_2() is invoked while
NEW_VTOKEN_IMPLEMENTATION placeholders remain unresolved for Ethereum, Arbitrum,
opBNB, Unichain and ZkSync; update the constants/fields in the vip-608
implementation (where NEW_VTOKEN_IMPLEMENTATION is defined) to the verified,
on-chain implementation addresses that match the venus-protocol-documentation,
then re-run testVip("VIP-608 Isolated Pool VToken Upgrade (cross-chain
dispatch)", await vip608_2()); ensure each chain-specific implementation address
in vip608_2() is replaced with the verified address and that the module exports
the fully populated payload before calling testVip().
In `@simulations/vip-608/bscmainnet.ts`:
- Around line 164-166: Replace the non-revert assertion with an explicit
success-code check: locate tests that call vToken.callStatic.accrueInterest()
(e.g., the it block in the simulation tests) and change the assertion from
expecting no revert to asserting the returned uint256 equals 0 by calling await
vToken.callStatic.accrueInterest() and comparing it with 0; apply this update to
all duplicated instances across the eight simulation test files (bscmainnet.ts,
zksyncmainnet.ts, unichainmainnet.ts, opmainnet.ts, opbnbmainnet.ts,
ethereum.ts, basemainnet.ts, arbitrumone.ts) so each test uses expect(await
vToken.callStatic.accrueInterest()).to.equal(0).
In `@vips/vip-608/addresses/arbitrumone.ts`:
- Line 7: ARBITRUMONE_NEW_VTOKEN_IMPLEMENTATION is currently set to the
placeholder "TODO", which will break proposal/simulation when treated as an
on-chain address; replace this placeholder with the verified deployed
implementation address (or remove/disable usage if not deployed) by updating the
constant ARBITRUMONE_NEW_VTOKEN_IMPLEMENTATION to the actual on-chain address
string that matches the verified source, ensuring the implementation contract is
verified and the address matches the venus-protocol-documentation records before
merging.
- Around line 3-7: Replace the hardcoded ARBITRUMONE_* address constants with
values imported from NETWORK_ADDRESSES: import NETWORK_ADDRESSES from
src/networkAddresses.ts and set ARBITRUMONE_ACM, ARBITRUMONE_NORMAL_TIMELOCK,
ARBITRUMONE_CORE_COMPTROLLER, ARBITRUMONE_VTOKEN_BEACON and
ARBITRUMONE_NEW_VTOKEN_IMPLEMENTATION to
NETWORK_ADDRESSES.arbitrumone.<appropriateKey> (use the matching keys in
networkAddresses for acm, normalTimelock, coreComptroller, vtokenBeacon,
newVTokenImplementation), keeping the same exported constant names.
In `@vips/vip-608/addresses/basemainnet.ts`:
- Around line 3-8: Replace the hard-coded base-mainnet address constants
(BASEMAINNET_ACM, BASEMAINNET_NORMAL_TIMELOCK, BASEMAINNET_CORE_COMPTROLLER,
BASEMAINNET_VTOKEN_BEACON, BASEMAINNET_NEW_VTOKEN_IMPLEMENTATION,
BASEMAINNET_DST_CHAIN_ID) so they are sourced from the exported
NETWORK_ADDRESSES structure in src/networkAddresses.ts (e.g. use
NETWORK_ADDRESSES.basemainnet.<key> for each corresponding value) and add the
necessary import for NETWORK_ADDRESSES at the top of the module.
In `@vips/vip-608/addresses/ethereum.ts`:
- Line 7: Replace the placeholder ETHEREUM_NEW_VTOKEN_IMPLEMENTATION string with
the real deployed contract address (use the checksummed hex form) and ensure
this address corresponds to the verified on-chain implementation whose source
matches the proposed code; update the constant
ETHEREUM_NEW_VTOKEN_IMPLEMENTATION with that checksummed address, confirm the
deployed bytecode and verified source match, and cross-check the address against
the venus-protocol-documentation to ensure it’s the correct verified
implementation before merging.
- Around line 20-21: ETHEREUM_CORE_VTOKENS currently includes "0x256add..."
(vEIGEN) and "0x325ceb..." (veBTC) but the comment says they have "different
proxy" while the code applies ETHEREUM_VTOKEN_BEACON upgrades and calls
upgradeTo/syncCash() uniformly; update the code so these two tokens are handled
correctly: either remove them from ETHEREUM_CORE_VTOKENS (so they are not
upgraded via ETHEREUM_VTOKEN_BEACON), or add explicit separate beacon addresses
and a mapping that uses those beacons (following the vip-582 pattern) and ensure
the upgradeTo call targets the correct beacon for each token, or if they
actually use the shared beacon just update the comments to state that; locate
ETHEREUM_CORE_VTOKENS, ETHEREUM_VTOKEN_BEACON, and any upgradeTo/syncCash()
invocation and apply one of these fixes so upgrades and syncCash() operate on
the correct proxy.
In `@vips/vip-608/addresses/opbnbmainnet.ts`:
- Line 7: Replace the placeholder string in the
OPBNBMAINNET_NEW_VTOKEN_IMPLEMENTATION constant with the actual deployed OPBNB
implementation contract address (use the checksummed/validated address format),
ensuring the on-chain address and the verified source code match the
implementation in the venus-protocol-documentation repository; update the
constant OPBNBMAINNET_NEW_VTOKEN_IMPLEMENTATION accordingly and re-run any
verification/tests that assert implementation equality to guarantee production
readiness.
- Around line 3-7: Replace the hard-coded OPBNBMAINNET_* address literals with
the canonical imports from NETWORK_ADDRESSES: import NETWORK_ADDRESSES from
src/networkAddresses.ts and use NETWORK_ADDRESSES.opbnbmainnet.<KEY> for
OPBNBMAINNET_ACM, OPBNBMAINNET_NORMAL_TIMELOCK, OPBNBMAINNET_CORE_COMPTROLLER,
OPBNBMAINNET_VTOKEN_BEACON and the NEW_VTOKEN_IMPLEMENTATION entry (leave as
TODO only if NETWORK_ADDRESSES.opbnbmainnet.newVtokenImplementation is
undefined); update the exported constants (OPBNBMAINNET_ACM,
OPBNBMAINNET_NORMAL_TIMELOCK, OPBNBMAINNET_CORE_COMPTROLLER,
OPBNBMAINNET_VTOKEN_BEACON, OPBNBMAINNET_NEW_VTOKEN_IMPLEMENTATION) to return
those NETWORK_ADDRESSES values.
In `@vips/vip-608/addresses/opmainnet.ts`:
- Around line 3-8: Replace the hardcoded OPMAINNET_* address literals by
importing NETWORK_ADDRESSES from src/networkAddresses.ts and assigning each
constant from NETWORK_ADDRESSES.opmainnet.<key> (e.g. replace OPMAINNET_ACM
literal with NETWORK_ADDRESSES.opmainnet.ACM, OPMAINNET_NORMAL_TIMELOCK with
NETWORK_ADDRESSES.opmainnet.NORMAL_TIMELOCK, OPMAINNET_CORE_COMPTROLLER with
NETWORK_ADDRESSES.opmainnet.CORE_COMPTROLLER, OPMAINNET_VTOKEN_BEACON with
NETWORK_ADDRESSES.opmainnet.VTOKEN_BEACON, and
OPMAINNET_NEW_VTOKEN_IMPLEMENTATION with
NETWORK_ADDRESSES.opmainnet.NEW_VTOKEN_IMPLEMENTATION); keep
OPMAINNET_DST_CHAIN_ID as LzChainId.opmainnet if it’s already using the chain
enum. Ensure you add the import for NETWORK_ADDRESSES at the top of the module.
In `@vips/vip-608/addresses/unichainmainnet.ts`:
- Line 7: The constant UNICHAINMAINNET_NEW_VTOKEN_IMPLEMENTATION is set to the
placeholder "TODO"; replace it with the actual deployed contract address for the
new vToken implementation (the verified on‑chain address) and ensure the address
corresponds to the verified source code; update
UNICHAINMAINNET_NEW_VTOKEN_IMPLEMENTATION to that checksum address and
cross-check it against the venus-protocol-documentation verified addresses to
guarantee the implementation matches on‑chain source.
- Around line 3-7: Replace the hard-coded constants UNICHAINMAINNET_ACM,
UNICHAINMAINNET_NORMAL_TIMELOCK, UNICHAINMAINNET_CORE_COMPTROLLER,
UNICHAINMAINNET_VTOKEN_BEACON, and UNICHAINMAINNET_NEW_VTOKEN_IMPLEMENTATION
with imports from the canonical NETWORK_ADDRESSES export: import
NETWORK_ADDRESSES from src/networkAddresses.ts and set each constant to the
corresponding NETWORK_ADDRESSES.<network>.acm / .normalTimelock /
.coreComptroller / .vTokenBeacon / .newVTokenImplementation key (use the exact
key names used in NETWORK_ADDRESSES for the unichainmainnet entry); keep the
same exported constant names but assign them from NETWORK_ADDRESSES instead of
literal strings so the file no longer duplicates address data.
In `@vips/vip-608/addresses/zksyncmainnet.ts`:
- Line 7: Replace the placeholder ZKSYNCMAINNET_NEW_VTOKEN_IMPLEMENTATION value
("TODO") with the actual deployed vToken implementation address; update the
constant ZKSYNCMAINNET_NEW_VTOKEN_IMPLEMENTATION to the on-chain address for the
new implementation, verify the implementation bytecode and source on the block
explorer and ensure the address/source match the venus-protocol-documentation
repo before committing.
- Around line 3-7: Replace the hardcoded constants ZKSYNCMAINNET_ACM,
ZKSYNCMAINNET_NORMAL_TIMELOCK, ZKSYNCMAINNET_CORE_COMPTROLLER,
ZKSYNCMAINNET_VTOKEN_BEACON and ZKSYNCMAINNET_NEW_VTOKEN_IMPLEMENTATION with
values imported from NETWORK_ADDRESSES; add an import for NETWORK_ADDRESSES from
src/networkAddresses.ts and set each exported constant to
NETWORK_ADDRESSES.<network>.<key> (e.g. NETWORK_ADDRESSES.zksyncmainnet.acm,
.normalTimelock, .coreComptroller, .vTokenBeacon, .newVTokenImplementation)
instead of the literal strings so the module follows the repository convention.
In `@vips/vip-608/bscmainnet-2.ts`:
- Around line 72-80: The Ethereum entry currently uses ETHEREUM_CORE_VTOKENS
(which still includes vEIGEN and veBTC) but only upgrades
ETHEREUM_VTOKEN_BEACON, so markets using a different proxy won't be moved before
syncCash(); update the Ethereum handling to exclude those markets from the
beacon-upgrade path: either remove vEIGEN and veBTC from ETHEREUM_CORE_VTOKENS
or add conditional filtering where the Ethereum config is processed to skip
addresses for vEIGEN and veBTC, and instead handle them via a separate
proxy-upgrade path (or separate config entry) so they are not targeted by the
ETHEREUM_VTOKEN_BEACON upgrade before syncCash() is dispatched. Ensure
references to ETHEREUM_CORE_VTOKENS, vEIGEN, veBTC, ETHEREUM_VTOKEN_BEACON and
syncCash are used to find the relevant code.
- Around line 72-134: The payload uses placeholder variables for new
implementation addresses—ETHEREUM_NEW_VTOKEN_IMPLEMENTATION,
ARBITRUMONE_NEW_VTOKEN_IMPLEMENTATION, OPBNBMAINNET_NEW_VTOKEN_IMPLEMENTATION,
UNICHAINMAINNET_NEW_VTOKEN_IMPLEMENTATION, and
ZKSYNCMAINNET_NEW_VTOKEN_IMPLEMENTATION—so the later upgradeTo(address) calls
will be invalid; replace these placeholders with the actual deployed
implementation addresses (or import the correct constants from the address
modules) before building the payload, ensure the properties newImplementation on
each chain entry contain valid checksummed addresses, and verify the upgrade
flow that calls upgradeTo(address) consumes these resolved values.
- Around line 4-59: Replace the many per-network address imports (e.g.,
ARBITRUMONE_ACM, ARBITRUMONE_VTOKEN_BEACON, BASEMAINNET_ACM,
BASEMAINNET_VTOKEN_BEACON, ETHEREUM_ACM, OPMAINNET_ACM, UNICHAINMAINNET_ACM,
ZKSYNCMAINNET_ACM, OPBNBMAINNET_ACM, their corresponding *_CORE_VTOKENS,
*_DST_CHAIN_ID, *_NEW_VTOKEN_IMPLEMENTATION, *_NORMAL_TIMELOCK symbols) with a
single import of NETWORK_ADDRESSES from src/networkAddresses.ts and reference
addresses as NETWORK_ADDRESSES.<network>.<key> (for example
NETWORK_ADDRESSES.arbitrumone.acm, .vtokenBeacon, .coreVtokens, .dstChainId,
.newVtokenImplementation, .normalTimelock); update all usages in this file
(vips/vip-608/bscmainnet-2.ts) to use those NETWORK_ADDRESSES lookups and remove
the old ./addresses/* imports.
In `@vips/vip-608/bscmainnet.ts`:
- Around line 52-53: MARKETS currently hardcodes excessAmount: "0" for every
market which forces sweepTokenAndSync(0) and can initialize internalCash
incorrectly; remove the blanket "0" defaults and either (A) populate
MARKETS.excessAmount by querying on-chain balances before calling
sweepTokenAndSync (add a helper that calls the market/internalCash read RPC and
fills MARKETS) or (B) remove excessAmount fields entirely and change
sweepTokenAndSync/internalCash logic to treat a missing/undefined excessAmount
as "unknown" and skip sweeping until an explicit verified numeric amount is
provided; update references to MARKETS, sweepTokenAndSync, and internalCash
accordingly and apply the same fix for entries in the 60-104 range.
- Around line 4-50: This file hardcodes BSC addresses (NEW_VBEP20_DELEGATE_IMPL
and all v* constants) instead of using the repo canonical registry; replace each
hardcoded constant with imports from NETWORK_ADDRESSES (e.g., use
NETWORK_ADDRESSES.bsc.NEW_VBEP20_DELEGATE_IMPL and NETWORK_ADDRESSES.bsc.vUSDC,
vUSDT, etc.), adding any missing keys to src/networkAddresses.ts first, then
remove the local constants so the proposal reads addresses from
NETWORK_ADDRESSES.<network>.<key>.
---
Nitpick comments:
In `@simulations/vip-608/bscmainnet.ts`:
- Around line 119-129: The test currently captures totalBorrows, totalReserves,
totalSupply and exchangeRateStored into preVipSnapshots for each
MARKET_ADDRESSES entry but only asserts supply later, so extend the post-VIP
assertions to compare all captured fields: after the VIP run iterate
MARKET_ADDRESSES and for each address retrieve the current
vToken.totalBorrows(), vToken.totalReserves(), vToken.totalSupply() and
vToken.exchangeRateStored() and assert equality against the values stored in
preVipSnapshots (use the same keys: totalBorrows, totalReserves, totalSupply,
exchangeRate) so regressions like sweepTokenAndSync that only mutate other
accounting fields fail; apply the same expanded assertions to the second
snapshot block referenced around the 159-162 area.
- Around line 15-106: Replace the two hardcoded arrays MARKET_ADDRESSES and
MARKET_NAMES with a single shared data source that pairs name and vToken (e.g.,
an array of { name, vToken } objects) and import network addresses from
src/networkAddresses.ts using NETWORK_ADDRESSES.<network>.<key> for each vToken;
update any code that referenced MARKET_ADDRESSES or MARKET_NAMES to use the new
combined structure (look for usages of MARKET_ADDRESSES, MARKET_NAMES, and any
place that iterates by index) so the test reads names and addresses from the
same authoritative source instead of relying on index alignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eda5bb47-c822-4662-9472-0e1052e5f67f
📒 Files selected for processing (23)
simulations/vip-608/abi/ILComptroller.jsonsimulations/vip-608/abi/ILPoolRegistry.jsonsimulations/vip-608/abi/ILVToken.jsonsimulations/vip-608/abi/VBep20Delegate.jsonsimulations/vip-608/abi/vtokenBeacon.jsonsimulations/vip-608/arbitrumone.tssimulations/vip-608/basemainnet.tssimulations/vip-608/bscmainnet-2.tssimulations/vip-608/bscmainnet.tssimulations/vip-608/ethereum.tssimulations/vip-608/opbnbmainnet.tssimulations/vip-608/opmainnet.tssimulations/vip-608/unichainmainnet.tssimulations/vip-608/zksyncmainnet.tsvips/vip-608/addresses/arbitrumone.tsvips/vip-608/addresses/basemainnet.tsvips/vip-608/addresses/ethereum.tsvips/vip-608/addresses/opbnbmainnet.tsvips/vip-608/addresses/opmainnet.tsvips/vip-608/addresses/unichainmainnet.tsvips/vip-608/addresses/zksyncmainnet.tsvips/vip-608/bscmainnet-2.tsvips/vip-608/bscmainnet.ts
Comment on lines
+3
to
+8
| import vip608_2 from "../../vips/vip-608/bscmainnet-2"; | ||
|
|
||
| const BLOCK_NUMBER = 87425358; | ||
|
|
||
| forking(BLOCK_NUMBER, async () => { | ||
| testVip("VIP-608 Isolated Pool VToken Upgrade (cross-chain dispatch)", await vip608_2()); |
There was a problem hiding this comment.
Resolve the remaining NEW_VTOKEN_IMPLEMENTATION placeholders before using vip608_2() here.
PR context still leaves Ethereum, Arbitrum, opBNB, Unichain, and ZkSync implementation addresses unresolved. Because testVip() has to build the full BSC proposal payload, a single placeholder address breaks this smoke test before the cross-chain dispatch path is exercised. This also leaves the remote suites that reuse vip608_2() non-actionable in this draft.
Based on learnings, "New or modified contracts must be verified: implementation must match on-chain source code, and addresses must match venus-protocol-documentation repo."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@simulations/vip-608/bscmainnet-2.ts` around lines 3 - 8, The test fails
because vip608_2() is invoked while NEW_VTOKEN_IMPLEMENTATION placeholders
remain unresolved for Ethereum, Arbitrum, opBNB, Unichain and ZkSync; update the
constants/fields in the vip-608 implementation (where NEW_VTOKEN_IMPLEMENTATION
is defined) to the verified, on-chain implementation addresses that match the
venus-protocol-documentation, then re-run testVip("VIP-608 Isolated Pool VToken
Upgrade (cross-chain dispatch)", await vip608_2()); ensure each chain-specific
implementation address in vip608_2() is replaced with the verified address and
that the module exports the fully populated payload before calling testVip().
Contributor
Author
There was a problem hiding this comment.
@coderabbitai The "TODO" placeholders for Ethereum, Arbitrum, opBNB, Unichain, and ZkSync are intentional — the new VToken implementation contracts haven't been deployed to those networks yet. These will be replaced with verified on-chain addresses once deployments are complete
There was a problem hiding this comment.
@Debugger022, understood — thanks for the clarification. Since this is a draft PR, having placeholder "TODO" addresses for not-yet-deployed contracts is a reasonable workflow.
A couple of things worth keeping in mind before promoting this to a ready state:
-
Runtime failures at test time:
ethers.jswill throw when it tries to encode"TODO"as anaddressparameter in the proposal payload (e.g., in theupgradeTo("TODO")call insidevip608_2()). This meansbscmainnet-2.tsand all the per-network simulation files (ethereum.ts,arbitrumone.ts,opbnbmainnet.ts,unichainmainnet.ts,zksyncmainnet.ts) will throw at construction time, not just fail assertions. Consider either:- Adding a guard at the top of the affected simulation files (e.g.,
if (NEW_VTOKEN_IMPLEMENTATION === "TODO") { return; }) so they gracefully skip until the address is filled in. - Or, conditionally omitting networks with
"TODO"implementations from theNETWORKSarray invip608_2()so the BSC cross-chain dispatch smoke test remains runnable against the already-deployed networks (Optimism and Base).
- Adding a guard at the top of the affected simulation files (e.g.,
-
Before merging: Ensure each
NEW_VTOKEN_IMPLEMENTATIONconstant is replaced with the verified on-chain address, and that the implementation source code matches thevenus-protocol-documentationrepo per the project's verification requirements.
🧠 Learnings used
Learnt from: CR
Repo: VenusProtocol/vips PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T08:08:09.287Z
Learning: Applies to simulations/**/simulations.ts : For remote (non-BSC) chain simulations, use testForkedNetworkVipCommands instead of testVip
Learnt from: CR
Repo: VenusProtocol/vips PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T08:08:09.287Z
Learning: Applies to simulations/**/simulations.ts : Simulation files must use forking() and testVip() utilities from src/vip-framework
Learnt from: CR
Repo: VenusProtocol/vips PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T08:08:09.287Z
Learning: Applies to multisig/simulations/**/*.ts : Multisig simulation tests must be located alongside proposal definitions in multisig/simulations/<network>/vip-NNN/
Learnt from: CR
Repo: VenusProtocol/vips PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T08:08:09.287Z
Learning: Applies to vips/*.ts : Cross-chain commands use dstChainId (LayerZero chain ID) to automatically route through OmnichainProposalSender on BSC
Learnt from: CR
Repo: VenusProtocol/vips PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T08:08:09.287Z
Learning: Applies to vips/*.ts : VIP proposal Command objects must include target, signature, and params; optionally value and dstChainId for cross-chain routing
Learnt from: CR
Repo: VenusProtocol/vips PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T08:08:09.287Z
Learning: Applies to vips/*.ts : VIP files must export a default function that returns a Proposal object
Learnt from: CR
Repo: VenusProtocol/vips PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T08:08:09.287Z
Learning: New or modified contracts must be verified: implementation must match on-chain source code, and addresses must match venus-protocol-documentation repo
Learnt from: CR
Repo: VenusProtocol/vips PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T08:08:09.287Z
Learning: Applies to vips/*.ts : Pending owner for contracts should be set to the appropriate timelock address (Normal, FastTrack, or Critical)
Learnt from: CR
Repo: VenusProtocol/vips PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T08:08:09.287Z
Learning: Applies to vips/*.ts : Function signatures in VIP Command objects must match the actual contract ABI for the target address
Comment on lines
+164
to
+166
| it("should allow accrueInterest to succeed", async () => { | ||
| // accrueInterest should work correctly with synced internalCash | ||
| await expect(vToken.callStatic.accrueInterest()).to.not.be.reverted; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== accrueInterest ABI outputs ==="
for file in simulations/vip-608/abi/ILVToken.json simulations/vip-608/abi/VBep20Delegate.json; do
echo "-- $file"
jq '.[] | select(.type=="function" and .name=="accrueInterest") | {name, outputs}' "$file"
done
echo
echo "=== matcher usage ==="
rg -n 'callStatic\.accrueInterest\(\)' simulations/vip-608Repository: VenusProtocol/vips
Length of output: 1421
Assert that accrueInterest() returns 0 (success code) instead of only checking non-revert.
The accrueInterest() function returns a uint256 error code (0 for success, non-zero for failure). Using only to.not.be.reverted verifies the call didn't throw, but a failing error code will still pass this assertion. Change the assertion to expect(await vToken.callStatic.accrueInterest()).to.equal(0).
This pattern is duplicated across 8 test files in simulations/vip-608/: bscmainnet.ts, zksyncmainnet.ts, unichainmainnet.ts, opmainnet.ts, opbnbmainnet.ts, ethereum.ts, basemainnet.ts, and arbitrumone.ts. All instances must be updated to properly validate success.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@simulations/vip-608/bscmainnet.ts` around lines 164 - 166, Replace the
non-revert assertion with an explicit success-code check: locate tests that call
vToken.callStatic.accrueInterest() (e.g., the it block in the simulation tests)
and change the assertion from expecting no revert to asserting the returned
uint256 equals 0 by calling await vToken.callStatic.accrueInterest() and
comparing it with 0; apply this update to all duplicated instances across the
eight simulation test files (bscmainnet.ts, zksyncmainnet.ts,
unichainmainnet.ts, opmainnet.ts, opbnbmainnet.ts, ethereum.ts, basemainnet.ts,
arbitrumone.ts) so each test uses expect(await
vToken.callStatic.accrueInterest()).to.equal(0).
Comment on lines
+3
to
+7
| export const ARBITRUMONE_ACM = "0xD9dD18EB0cf10CbA837677f28A8F9Bda4bc2b157"; | ||
| export const ARBITRUMONE_NORMAL_TIMELOCK = "0x4b94589Cc23F618687790036726f744D602c4017"; | ||
| export const ARBITRUMONE_CORE_COMPTROLLER = "0x317c1A5739F39046E20b08ac9BeEa3f10fD43326"; | ||
| export const ARBITRUMONE_VTOKEN_BEACON = "0xE9381D8CA7006c12Ae9eB97890575E705996fa66"; | ||
| export const ARBITRUMONE_NEW_VTOKEN_IMPLEMENTATION = "TODO"; // Not yet deployed |
There was a problem hiding this comment.
Use NETWORK_ADDRESSES instead of hardcoded address literals.
Line 3 to Line 7 should source addresses from src/networkAddresses.ts to keep a single source of truth and avoid drift.
As per coding guidelines Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES.<network>.<key>.
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vips/vip-608/addresses/arbitrumone.ts` around lines 3 - 7, Replace the
hardcoded ARBITRUMONE_* address constants with values imported from
NETWORK_ADDRESSES: import NETWORK_ADDRESSES from src/networkAddresses.ts and set
ARBITRUMONE_ACM, ARBITRUMONE_NORMAL_TIMELOCK, ARBITRUMONE_CORE_COMPTROLLER,
ARBITRUMONE_VTOKEN_BEACON and ARBITRUMONE_NEW_VTOKEN_IMPLEMENTATION to
NETWORK_ADDRESSES.arbitrumone.<appropriateKey> (use the matching keys in
networkAddresses for acm, normalTimelock, coreComptroller, vtokenBeacon,
newVTokenImplementation), keeping the same exported constant names.
Comment on lines
+3
to
+8
| export const BASEMAINNET_ACM = "0x9E6CeEfDC6183e4D0DF8092A9B90cDF659687daB"; | ||
| export const BASEMAINNET_NORMAL_TIMELOCK = "0x21c12f2946a1a66cBFf7eb997022a37167eCf517"; | ||
| export const BASEMAINNET_CORE_COMPTROLLER = "0x0C7973F9598AA62f9e03B94E92C967fD5437426C"; | ||
| export const BASEMAINNET_VTOKEN_BEACON = "0x87a6476510368c4Bfb70d04A3B0e5a881eC7f0d1"; | ||
| export const BASEMAINNET_NEW_VTOKEN_IMPLEMENTATION = "0x867050A5eE96Af41D62160F6e1F4724828eD3D6e"; | ||
| export const BASEMAINNET_DST_CHAIN_ID = LzChainId.basemainnet; |
There was a problem hiding this comment.
Base mainnet constants should come from src/networkAddresses.ts.
Line 3 to Line 8 should reference NETWORK_ADDRESSES instead of embedded literals.
As per coding guidelines Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES.<network>.<key>.
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 7-7: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vips/vip-608/addresses/basemainnet.ts` around lines 3 - 8, Replace the
hard-coded base-mainnet address constants (BASEMAINNET_ACM,
BASEMAINNET_NORMAL_TIMELOCK, BASEMAINNET_CORE_COMPTROLLER,
BASEMAINNET_VTOKEN_BEACON, BASEMAINNET_NEW_VTOKEN_IMPLEMENTATION,
BASEMAINNET_DST_CHAIN_ID) so they are sourced from the exported
NETWORK_ADDRESSES structure in src/networkAddresses.ts (e.g. use
NETWORK_ADDRESSES.basemainnet.<key> for each corresponding value) and add the
necessary import for NETWORK_ADDRESSES at the top of the module.
Comment on lines
+4
to
+59
| import { | ||
| ARBITRUMONE_ACM, | ||
| ARBITRUMONE_CORE_VTOKENS, | ||
| ARBITRUMONE_DST_CHAIN_ID, | ||
| ARBITRUMONE_NEW_VTOKEN_IMPLEMENTATION, | ||
| ARBITRUMONE_NORMAL_TIMELOCK, | ||
| ARBITRUMONE_VTOKEN_BEACON, | ||
| } from "./addresses/arbitrumone"; | ||
| import { | ||
| BASEMAINNET_ACM, | ||
| BASEMAINNET_CORE_VTOKENS, | ||
| BASEMAINNET_DST_CHAIN_ID, | ||
| BASEMAINNET_NEW_VTOKEN_IMPLEMENTATION, | ||
| BASEMAINNET_NORMAL_TIMELOCK, | ||
| BASEMAINNET_VTOKEN_BEACON, | ||
| } from "./addresses/basemainnet"; | ||
| import { | ||
| ETHEREUM_ACM, | ||
| ETHEREUM_CORE_VTOKENS, | ||
| ETHEREUM_DST_CHAIN_ID, | ||
| ETHEREUM_NEW_VTOKEN_IMPLEMENTATION, | ||
| ETHEREUM_NORMAL_TIMELOCK, | ||
| ETHEREUM_VTOKEN_BEACON, | ||
| } from "./addresses/ethereum"; | ||
| import { | ||
| OPBNBMAINNET_ACM, | ||
| OPBNBMAINNET_CORE_VTOKENS, | ||
| OPBNBMAINNET_DST_CHAIN_ID, | ||
| OPBNBMAINNET_NEW_VTOKEN_IMPLEMENTATION, | ||
| OPBNBMAINNET_NORMAL_TIMELOCK, | ||
| OPBNBMAINNET_VTOKEN_BEACON, | ||
| } from "./addresses/opbnbmainnet"; | ||
| import { | ||
| OPMAINNET_ACM, | ||
| OPMAINNET_CORE_VTOKENS, | ||
| OPMAINNET_DST_CHAIN_ID, | ||
| OPMAINNET_NEW_VTOKEN_IMPLEMENTATION, | ||
| OPMAINNET_NORMAL_TIMELOCK, | ||
| OPMAINNET_VTOKEN_BEACON, | ||
| } from "./addresses/opmainnet"; | ||
| import { | ||
| UNICHAINMAINNET_ACM, | ||
| UNICHAINMAINNET_CORE_VTOKENS, | ||
| UNICHAINMAINNET_DST_CHAIN_ID, | ||
| UNICHAINMAINNET_NEW_VTOKEN_IMPLEMENTATION, | ||
| UNICHAINMAINNET_NORMAL_TIMELOCK, | ||
| UNICHAINMAINNET_VTOKEN_BEACON, | ||
| } from "./addresses/unichainmainnet"; | ||
| import { | ||
| ZKSYNCMAINNET_ACM, | ||
| ZKSYNCMAINNET_CORE_VTOKENS, | ||
| ZKSYNCMAINNET_DST_CHAIN_ID, | ||
| ZKSYNCMAINNET_NEW_VTOKEN_IMPLEMENTATION, | ||
| ZKSYNCMAINNET_NORMAL_TIMELOCK, | ||
| ZKSYNCMAINNET_VTOKEN_BEACON, | ||
| } from "./addresses/zksyncmainnet"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use NETWORK_ADDRESSES as the single source of truth for these targets.
This duplicates ACM, timelock, beacon, implementation, and market addresses through local ./addresses/* modules, which makes governance targets harder to audit and easier to let drift across proposal and simulation code.
As per coding guidelines, "Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES..".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vips/vip-608/bscmainnet-2.ts` around lines 4 - 59, Replace the many
per-network address imports (e.g., ARBITRUMONE_ACM, ARBITRUMONE_VTOKEN_BEACON,
BASEMAINNET_ACM, BASEMAINNET_VTOKEN_BEACON, ETHEREUM_ACM, OPMAINNET_ACM,
UNICHAINMAINNET_ACM, ZKSYNCMAINNET_ACM, OPBNBMAINNET_ACM, their corresponding
*_CORE_VTOKENS, *_DST_CHAIN_ID, *_NEW_VTOKEN_IMPLEMENTATION, *_NORMAL_TIMELOCK
symbols) with a single import of NETWORK_ADDRESSES from src/networkAddresses.ts
and reference addresses as NETWORK_ADDRESSES.<network>.<key> (for example
NETWORK_ADDRESSES.arbitrumone.acm, .vtokenBeacon, .coreVtokens, .dstChainId,
.newVtokenImplementation, .normalTimelock); update all usages in this file
(vips/vip-608/bscmainnet-2.ts) to use those NETWORK_ADDRESSES lookups and remove
the old ./addresses/* imports.
Comment on lines
+4
to
+50
| // TODO: Replace with the deployed VBep20Delegate implementation address | ||
| const NEW_VBEP20_DELEGATE_IMPL = "0xb25b57599BA969c4829699F7E4Fc4076D14745E1"; | ||
|
|
||
| // VBep20Delegator proxy addresses (43 markets, excludes vBNB) | ||
| const vUSDC = "0xecA88125a5ADbe82614ffC12D0DB554E2e2867C8"; | ||
| const vUSDT = "0xfD5840Cd36d94D7229439859C0112a4185BC0255"; | ||
| const vBUSD = "0x95c78222B3D6e262426483D42CfA53685A67Ab9D"; | ||
| const vSXP = "0x2fF3d0F6990a40261c66E1ff2017aCBc282EB6d0"; | ||
| const vXVS = "0x151B1e2635A717bcDc836ECd6FbB62B674FE3E1D"; | ||
| const vBTC = "0x882C173bC7Ff3b7786CA16dfeD3DFFfb9Ee7847B"; | ||
| const vETH = "0xf508fCD89b8bd15579dc79A6827cB4686A3592c8"; | ||
| const vLTC = "0x57A5297F2cB2c0AaC9D554660acd6D385Ab50c6B"; | ||
| const vXRP = "0xB248a295732e0225acd3337607cc01068e3b9c10"; | ||
| const vBCH = "0x5F0388EBc2B94FA8E123F404b79cCF5f40b29176"; | ||
| const vDOT = "0x1610bc33319e9398de5f57B33a5b184c806aD217"; | ||
| const vLINK = "0x650b940a1033B8A1b1873f78730FcFC73ec11f1f"; | ||
| const vDAI = "0x334b3eCB4DCa3593BCCC3c7EBD1A1C1d1780FBF1"; | ||
| const vFIL = "0xf91d58b5aE142DAcC749f58A49FCBac340Cb0343"; | ||
| const vBETH = "0x972207A639CC1B374B893cc33Fa251b55CEB7c07"; | ||
| const vADA = "0x9A0AF7FDb2065Ce470D72664DE73cAE409dA28Ec"; | ||
| const vDOGE = "0xec3422Ef92B2fb59e84c8B02Ba73F1fE84Ed8D71"; | ||
| const vMATIC = "0x5c9476FcD6a4F9a3654139721c949c2233bBbBc8"; | ||
| const vCAKE = "0x86aC3974e2BD0d60825230fa6F355fF11409df5c"; | ||
| const vAAVE = "0x26DA28954763B92139ED49283625ceCAf52C6f94"; | ||
| const vTUSDOLD = "0x08CEB3F4a7ed3500cA0982bcd0FC7816688084c3"; | ||
| const vTRXOLD = "0x61eDcFe8Dd6bA3c891CB9bEc2dc7657B3B422E93"; | ||
| const vTRX = "0xC5D3466aA484B040eE977073fcF337f2c00071c1"; | ||
| const vWBETH = "0x6CFdEc747f37DAf3b87a35a1D9c8AD3063A1A8A0"; | ||
| const vTUSD = "0xBf762cd5991cA1DCdDaC9ae5C638F5B5Dc3Bee6E"; | ||
| const vUNI = "0x27FF564707786720C71A2e5c1490A63266683612"; | ||
| const vFDUSD = "0xC4eF4229FEc74Ccfe17B2bdeF7715fAC740BA0ba"; | ||
| const vTWT = "0x4d41a36D04D97785bcEA57b057C412b278e6Edcc"; | ||
| const vSolvBTC = "0xf841cb62c19fCd4fF5CD0AaB5939f3140BaaC3Ea"; | ||
| const vTHE = "0x86e06EAfa6A1eA631Eab51DE500E3D474933739f"; | ||
| const vSOL = "0xBf515bA4D1b52FFdCeaBF20d31D705Ce789F2cEC"; | ||
| const vlisUSD = "0x689E0daB47Ab16bcae87Ec18491692BF621Dc6Ab"; | ||
| const vPT_sUSDE_26JUN2025 = "0x9e4E5fed5Ac5B9F732d0D850A615206330Bf1866"; | ||
| const vsUSDe = "0x699658323d58eE25c69F1a29d476946ab011bD18"; | ||
| const vUSDe = "0x74ca6930108F775CC667894EEa33843e691680d7"; | ||
| const vUSD1 = "0x0C1DA220D301155b87318B90692Da8dc43B67340"; | ||
| const vxSolvBTC = "0xd804dE60aFD05EE6B89aab5D152258fD461B07D5"; | ||
| const vasBNB = "0xCC1dB43a06d97f736C7B045AedD03C6707c09BDF"; | ||
| const vWBNB = "0x6bCa74586218dB34cdB402295796b79663d816e9"; | ||
| const vslisBNB = "0x89c910Eb8c90df818b4649b508Ba22130Dc73Adc"; | ||
| const vU = "0x3d5E269787d562b74aCC55F18Bd26C5D09Fa245E"; | ||
| const vPT_clisBNB_25JUN2026 = "0x6d3BD68E90B42615cb5abF4B8DE92b154ADc435e"; | ||
| const vXAUM = "0x92e6Ea74a1A3047DabF4186405a21c7D63a0612A"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move the BSC targets into NETWORK_ADDRESSES.
This file hardcodes the delegate implementation and every market address locally, so the proposal is no longer reading targets from the repo's canonical address registry. If some keys are missing there, please add them first and import them from this file.
As per coding guidelines, "Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES..".
Also applies to: 60-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vips/vip-608/bscmainnet.ts` around lines 4 - 50, This file hardcodes BSC
addresses (NEW_VBEP20_DELEGATE_IMPL and all v* constants) instead of using the
repo canonical registry; replace each hardcoded constant with imports from
NETWORK_ADDRESSES (e.g., use NETWORK_ADDRESSES.bsc.NEW_VBEP20_DELEGATE_IMPL and
NETWORK_ADDRESSES.bsc.vUSDC, vUSDT, etc.), adding any missing keys to
src/networkAddresses.ts first, then remove the local constants so the proposal
reads addresses from NETWORK_ADDRESSES.<network>.<key>.
Comment on lines
+52
to
+53
| // Markets and their excess token amounts to sweep before syncing | ||
| // TODO: Verify excess amounts on-chain before proposal. Use 0 for markets with no excess. |
There was a problem hiding this comment.
Don't ship excessAmount: "0" for every market while the on-chain check is still unresolved.
The file TODO says these balances still need verification, but MARKETS already drives sweepTokenAndSync(0) across all 43 markets. If any market has stray underlying, internalCash will be initialized from the wrong starting balance.
Also applies to: 60-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vips/vip-608/bscmainnet.ts` around lines 52 - 53, MARKETS currently hardcodes
excessAmount: "0" for every market which forces sweepTokenAndSync(0) and can
initialize internalCash incorrectly; remove the blanket "0" defaults and either
(A) populate MARKETS.excessAmount by querying on-chain balances before calling
sweepTokenAndSync (add a helper that calls the market/internalCash read RPC and
fills MARKETS) or (B) remove excessAmount fields entirely and change
sweepTokenAndSync/internalCash logic to treat a missing/undefined excessAmount
as "unknown" and skip sweeping until an explicit verified numeric amount is
provided; update references to MARKETS, sweepTokenAndSync, and internalCash
accordingly and apply the same fix for entries in the 60-104 range.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
vips/vip-608/addresses/opbnbmainnet.ts (1)
3-7:⚠️ Potential issue | 🟠 MajorUse canonical
NETWORK_ADDRESSESinstead of hard-coded OPBNB literals.These constants should be sourced from
src/networkAddresses.ts(NETWORK_ADDRESSES.opbnbmainnet.<key>) to avoid config drift across proposals/tests.As per coding guidelines
Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES.<network>.<key>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vips/vip-608/addresses/opbnbmainnet.ts` around lines 3 - 7, Replace the hard-coded OPBNBMAINNET_* address constants in this file with references to the canonical NETWORK_ADDRESSES object: import NETWORK_ADDRESSES from src/networkAddresses.ts and change OPBNBMAINNET_ACM, OPBNBMAINNET_NORMAL_TIMELOCK, OPBNBMAINNET_CORE_COMPTROLLER, OPBNBMAINNET_VTOKEN_BEACON, and OPBNBMAINNET_NEW_VTOKEN_IMPLEMENTATION to read their values from NETWORK_ADDRESSES.opbnbmainnet (e.g. NETWORK_ADDRESSES.opbnbmainnet.ACM, .NORMAL_TIMELOCK, .CORE_COMPTROLLER, .VTOKEN_BEACON, .NEW_VTOKEN_IMPLEMENTATION) so the file uses the central config instead of hard-coded literals.vips/vip-608/addresses/basemainnet.ts (1)
3-16:⚠️ Potential issue | 🟠 MajorReplace hardcoded Base addresses with
NETWORK_ADDRESSESlookups.These constants are still embedded literals; they should be sourced from
src/networkAddresses.ts(NETWORK_ADDRESSES.basemainnet.<key>) to comply with repo standards and avoid drift across VIP files.Suggested patch
import { LzChainId } from "src/types"; +import { NETWORK_ADDRESSES } from "src/networkAddresses"; -export const BASEMAINNET_ACM = "0x9E6CeEfDC6183e4D0DF8092A9B90cDF659687daB"; -export const BASEMAINNET_NORMAL_TIMELOCK = "0x21c12f2946a1a66cBFf7eb997022a37167eCf517"; -export const BASEMAINNET_CORE_COMPTROLLER = "0x0C7973F9598AA62f9e03B94E92C967fD5437426C"; -export const BASEMAINNET_VTOKEN_BEACON = "0x87a6476510368c4Bfb70d04A3B0e5a881eC7f0d1"; -export const BASEMAINNET_NEW_VTOKEN_IMPLEMENTATION = "0x107bA74c87e75b6cc291510D3A85D0c8EAa73e82"; +export const BASEMAINNET_ACM = NETWORK_ADDRESSES.basemainnet.ACM; +export const BASEMAINNET_NORMAL_TIMELOCK = NETWORK_ADDRESSES.basemainnet.NORMAL_TIMELOCK; +export const BASEMAINNET_CORE_COMPTROLLER = NETWORK_ADDRESSES.basemainnet.CORE_COMPTROLLER; +export const BASEMAINNET_VTOKEN_BEACON = NETWORK_ADDRESSES.basemainnet.VTOKEN_BEACON; +export const BASEMAINNET_NEW_VTOKEN_IMPLEMENTATION = NETWORK_ADDRESSES.basemainnet.NEW_VTOKEN_IMPLEMENTATION; export const BASEMAINNET_DST_CHAIN_ID = LzChainId.basemainnet;As per coding guidelines
Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES.<network>.<key>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vips/vip-608/addresses/basemainnet.ts` around lines 3 - 16, Replace hardcoded BaseMainnet address literals in BASEMAINNET_ACM, BASEMAINNET_NORMAL_TIMELOCK, BASEMAINNET_CORE_COMPTROLLER, BASEMAINNET_VTOKEN_BEACON, BASEMAINNET_NEW_VTOKEN_IMPLEMENTATION, BASEMAINNET_DST_CHAIN_ID and entries in BASEMAINNET_CORE_VTOKENS with lookups from NETWORK_ADDRESSES.basemainnet (e.g., NETWORK_ADDRESSES.basemainnet.acm, .normalTimelock, .coreComptroller, .vTokenBeacon, .newVTokenImplementation, .dstChainId and the specific vtoken keys); add an import for NETWORK_ADDRESSES from src/networkAddresses.ts and update each constant (and the array elements) to reference the corresponding NETWORK_ADDRESSES.basemainnet.<key> property instead of the string literals so the file follows the repo convention.vips/vip-608/addresses/unichainmainnet.ts (1)
3-7:⚠️ Potential issue | 🟠 MajorUse canonical
NETWORK_ADDRESSESinstead of hard-coded addresses.These constants duplicate address data locally; they should be sourced from
src/networkAddresses.tsto avoid drift across VIP files.♻️ Proposed change
import { LzChainId } from "src/types"; +import NETWORK_ADDRESSES from "src/networkAddresses"; -export const UNICHAINMAINNET_ACM = "0x1f12014c497a9d905155eB9BfDD9FaC6885e61d0"; -export const UNICHAINMAINNET_NORMAL_TIMELOCK = "0x918532A78d22419Da4091930d472bDdf532BE89a"; -export const UNICHAINMAINNET_CORE_COMPTROLLER = "0xe22af1e6b78318e1Fe1053Edbd7209b8Fc62c4Fe"; -export const UNICHAINMAINNET_VTOKEN_BEACON = "0x42c1Efb9Dd9424c5ac8e6EcEa4eb03940c4a15Fc"; -export const UNICHAINMAINNET_NEW_VTOKEN_IMPLEMENTATION = "0x256E61f4056EC9eaB8c56E3aFa217a12c33b8893"; +export const UNICHAINMAINNET_ACM = NETWORK_ADDRESSES.unichainmainnet.acm; +export const UNICHAINMAINNET_NORMAL_TIMELOCK = NETWORK_ADDRESSES.unichainmainnet.normalTimelock; +export const UNICHAINMAINNET_CORE_COMPTROLLER = NETWORK_ADDRESSES.unichainmainnet.coreComptroller; +export const UNICHAINMAINNET_VTOKEN_BEACON = NETWORK_ADDRESSES.unichainmainnet.vTokenBeacon; +export const UNICHAINMAINNET_NEW_VTOKEN_IMPLEMENTATION = NETWORK_ADDRESSES.unichainmainnet.newVTokenImplementation; export const UNICHAINMAINNET_DST_CHAIN_ID = LzChainId.unichainmainnet; -export const UNICHAINMAINNET_CORE_VTOKENS = [ - "0xc219bc179c7cdb37eacb03f993f9fdc2495e3374", // vWETH_Core - "0xb953f92b9f759d97d2f2dec10a8a3cf75fce3a95", // vUSDC_Core - "0x67716d6bf76170af816f5735e14c4d44d0b05ed2", // vUNI_Core - "0x0170398083eb0d0387709523bafca6426146c218", // vweETH_Core - "0xbec19bef402c697a7be315d3e59e5f65b89fa1bb", // vwstETH_Core - "0x68e2a6f7257fac2f5a557b9e83e1fe6d5b408ce5", // vWBTC_Core - "0xda7ce7ba016d266645712e2e4ebc6cc75ea8e4cd", // vUSD₮0_Core -]; +export const UNICHAINMAINNET_CORE_VTOKENS = NETWORK_ADDRESSES.unichainmainnet.coreVTokens;As per coding guidelines
Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES.<network>.<key>.Also applies to: 10-18
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vips/vip-608/addresses/unichainmainnet.ts` around lines 3 - 7, This file defines hard-coded constants (UNICHAINMAINNET_ACM, UNICHAINMAINNET_NORMAL_TIMELOCK, UNICHAINMAINNET_CORE_COMPTROLLER, UNICHAINMAINNET_VTOKEN_BEACON, UNICHAINMAINNET_NEW_VTOKEN_IMPLEMENTATION) that should be sourced from the canonical NETWORK_ADDRESSES; replace the local constants by importing NETWORK_ADDRESSES from src/networkAddresses.ts and re-exporting the values as e.g. export const UNICHAINMAINNET_ACM = NETWORK_ADDRESSES.UNICHAINMAINNET.ACM (and similarly map NORMAL_TIMELOCK, CORE_COMPTROLLER, VTOKEN_BEACON, NEW_VTOKEN_IMPLEMENTATION), and update/remove any other duplicated constants in this file (lines ~10-18) so all addresses come from NETWORK_ADDRESSES.<network>.<key>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@vips/vip-608/addresses/basemainnet.ts`:
- Around line 3-16: Replace hardcoded BaseMainnet address literals in
BASEMAINNET_ACM, BASEMAINNET_NORMAL_TIMELOCK, BASEMAINNET_CORE_COMPTROLLER,
BASEMAINNET_VTOKEN_BEACON, BASEMAINNET_NEW_VTOKEN_IMPLEMENTATION,
BASEMAINNET_DST_CHAIN_ID and entries in BASEMAINNET_CORE_VTOKENS with lookups
from NETWORK_ADDRESSES.basemainnet (e.g., NETWORK_ADDRESSES.basemainnet.acm,
.normalTimelock, .coreComptroller, .vTokenBeacon, .newVTokenImplementation,
.dstChainId and the specific vtoken keys); add an import for NETWORK_ADDRESSES
from src/networkAddresses.ts and update each constant (and the array elements)
to reference the corresponding NETWORK_ADDRESSES.basemainnet.<key> property
instead of the string literals so the file follows the repo convention.
In `@vips/vip-608/addresses/opbnbmainnet.ts`:
- Around line 3-7: Replace the hard-coded OPBNBMAINNET_* address constants in
this file with references to the canonical NETWORK_ADDRESSES object: import
NETWORK_ADDRESSES from src/networkAddresses.ts and change OPBNBMAINNET_ACM,
OPBNBMAINNET_NORMAL_TIMELOCK, OPBNBMAINNET_CORE_COMPTROLLER,
OPBNBMAINNET_VTOKEN_BEACON, and OPBNBMAINNET_NEW_VTOKEN_IMPLEMENTATION to read
their values from NETWORK_ADDRESSES.opbnbmainnet (e.g.
NETWORK_ADDRESSES.opbnbmainnet.ACM, .NORMAL_TIMELOCK, .CORE_COMPTROLLER,
.VTOKEN_BEACON, .NEW_VTOKEN_IMPLEMENTATION) so the file uses the central config
instead of hard-coded literals.
In `@vips/vip-608/addresses/unichainmainnet.ts`:
- Around line 3-7: This file defines hard-coded constants (UNICHAINMAINNET_ACM,
UNICHAINMAINNET_NORMAL_TIMELOCK, UNICHAINMAINNET_CORE_COMPTROLLER,
UNICHAINMAINNET_VTOKEN_BEACON, UNICHAINMAINNET_NEW_VTOKEN_IMPLEMENTATION) that
should be sourced from the canonical NETWORK_ADDRESSES; replace the local
constants by importing NETWORK_ADDRESSES from src/networkAddresses.ts and
re-exporting the values as e.g. export const UNICHAINMAINNET_ACM =
NETWORK_ADDRESSES.UNICHAINMAINNET.ACM (and similarly map NORMAL_TIMELOCK,
CORE_COMPTROLLER, VTOKEN_BEACON, NEW_VTOKEN_IMPLEMENTATION), and update/remove
any other duplicated constants in this file (lines ~10-18) so all addresses come
from NETWORK_ADDRESSES.<network>.<key>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d25b9d83-9d13-4af1-a88f-ed4263f996d7
📒 Files selected for processing (8)
vips/vip-608/addresses/arbitrumone.tsvips/vip-608/addresses/basemainnet.tsvips/vip-608/addresses/ethereum.tsvips/vip-608/addresses/opbnbmainnet.tsvips/vip-608/addresses/opmainnet.tsvips/vip-608/addresses/unichainmainnet.tsvips/vip-608/addresses/zksyncmainnet.tsvips/vip-608/bscmainnet.ts
✅ Files skipped from review due to trivial changes (3)
- vips/vip-608/addresses/arbitrumone.ts
- vips/vip-608/addresses/opmainnet.ts
- vips/vip-608/addresses/zksyncmainnet.ts
…proposal - Update NEW_VTOKEN_IMPLEMENTATION addresses for all remote chains - Split bscmainnet-2 cross-chain proposal to avoid LayerZero payload size limit - Add bscmainnet-3 for remaining cross-chain commands - Update simulation block numbers and test structure
There was a problem hiding this comment.
🧹 Nitpick comments (1)
simulations/vip-608/bscmainnet.ts (1)
16-106: Consider adding a runtime assertion to verify array length consistency.The
MARKET_ADDRESSESandMARKET_NAMESarrays are both expected to have 43 entries. A mismatch would cause incorrect market name associations in test output.🔧 Suggested validation
Add after line 106:
if (MARKET_ADDRESSES.length !== MARKET_NAMES.length) { throw new Error(`MARKET_ADDRESSES (${MARKET_ADDRESSES.length}) and MARKET_NAMES (${MARKET_NAMES.length}) length mismatch`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-608/bscmainnet.ts` around lines 16 - 106, Add a runtime assertion after the MARKET_ADDRESSES and MARKET_NAMES declarations to verify they have the same length: check MARKET_ADDRESSES.length !== MARKET_NAMES.length and throw a clear Error if mismatched so tests won't misassociate names; reference the MARKET_ADDRESSES and MARKET_NAMES arrays when adding this guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@simulations/vip-608/bscmainnet.ts`:
- Around line 16-106: Add a runtime assertion after the MARKET_ADDRESSES and
MARKET_NAMES declarations to verify they have the same length: check
MARKET_ADDRESSES.length !== MARKET_NAMES.length and throw a clear Error if
mismatched so tests won't misassociate names; reference the MARKET_ADDRESSES and
MARKET_NAMES arrays when adding this guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 041326ac-11db-4f88-add6-8a5fc8d7f80b
📒 Files selected for processing (12)
simulations/vip-608/arbitrumone.tssimulations/vip-608/basemainnet.tssimulations/vip-608/bscmainnet-2.tssimulations/vip-608/bscmainnet-3.tssimulations/vip-608/bscmainnet.tssimulations/vip-608/ethereum.tssimulations/vip-608/opbnbmainnet.tssimulations/vip-608/opmainnet.tssimulations/vip-608/unichainmainnet.tssimulations/vip-608/zksyncmainnet.tsvips/vip-608/bscmainnet-2.tsvips/vip-608/bscmainnet-3.ts
✅ Files skipped from review due to trivial changes (4)
- simulations/vip-608/bscmainnet-2.ts
- simulations/vip-608/basemainnet.ts
- simulations/vip-608/ethereum.ts
- simulations/vip-608/unichainmainnet.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- simulations/vip-608/opmainnet.ts
- simulations/vip-608/opbnbmainnet.ts
- vips/vip-608/bscmainnet-2.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
vips/vip-608/addresses/ethereum.ts (1)
3-34:⚠️ Potential issue | 🟠 MajorReplace hardcoded Ethereum addresses with
NETWORK_ADDRESSESsources.This file still embeds network addresses directly, which makes drift/regression more likely versus using centralized address sources.
As per coding guidelines
Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES.<network>.<key>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vips/vip-608/addresses/ethereum.ts` around lines 3 - 34, This module hardcodes Ethereum addresses (ETHEREUM_ACM, ETHEREUM_NORMAL_TIMELOCK, ETHEREUM_CORE_COMPTROLLER, ETHEREUM_VTOKEN_BEACON, ETHEREUM_NEW_VTOKEN_IMPLEMENTATION, and ETHEREUM_CORE_VTOKENS) instead of using the canonical NETWORK_ADDRESSES; import NETWORK_ADDRESSES from src/networkAddresses.ts and replace each constant assignment with the corresponding NETWORK_ADDRESSES.ethereum.<key> lookup (and convert the array ETHEREUM_CORE_VTOKENS to an array of NETWORK_ADDRESSES.ethereum.<tokenKey> entries), keeping ETHEREUM_DST_CHAIN_ID set to LzChainId.ethereum if needed.
🧹 Nitpick comments (1)
simulations/vip-608/ethereum.ts (1)
28-32: Strengthen market coverage assertion to compare sets, not only counts.The current check can pass even if one address is missing and another extra address is present.
Proposed test hardening
it("CORE_VTOKENS should cover all on-chain markets", async () => { const comptroller = new ethers.Contract(ETHEREUM_CORE_COMPTROLLER, COMPTROLLER_ABI, provider); const allMarkets: string[] = await comptroller.getAllMarkets(); - expect(ETHEREUM_CORE_VTOKENS.length).to.equal(allMarkets.length, "CORE_VTOKENS does not cover all markets"); + const normalize = (markets: string[]) => markets.map(m => m.toLowerCase()).sort(); + expect(normalize(ETHEREUM_CORE_VTOKENS)).to.deep.equal( + normalize(allMarkets), + "CORE_VTOKENS does not match on-chain markets", + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-608/ethereum.ts` around lines 28 - 32, Update the test "CORE_VTOKENS should cover all on-chain markets" to compare sets instead of only lengths: fetch on-chain markets via comptroller.getAllMarkets() into allMarkets and assert that every address in ETHEREUM_CORE_VTOKENS appears in allMarkets and vice versa (no extras/missing) by comparing sorted or Set representations (e.g., new Set(ETHEREUM_CORE_VTOKENS) vs new Set(allMarkets)); keep existing variables (comptroller, getAllMarkets, ETHEREUM_CORE_VTOKENS, allMarkets) and replace the length equality assertion with a set-equality check that fails if any address is missing or unexpected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@vips/vip-608/addresses/ethereum.ts`:
- Around line 3-34: This module hardcodes Ethereum addresses (ETHEREUM_ACM,
ETHEREUM_NORMAL_TIMELOCK, ETHEREUM_CORE_COMPTROLLER, ETHEREUM_VTOKEN_BEACON,
ETHEREUM_NEW_VTOKEN_IMPLEMENTATION, and ETHEREUM_CORE_VTOKENS) instead of using
the canonical NETWORK_ADDRESSES; import NETWORK_ADDRESSES from
src/networkAddresses.ts and replace each constant assignment with the
corresponding NETWORK_ADDRESSES.ethereum.<key> lookup (and convert the array
ETHEREUM_CORE_VTOKENS to an array of NETWORK_ADDRESSES.ethereum.<tokenKey>
entries), keeping ETHEREUM_DST_CHAIN_ID set to LzChainId.ethereum if needed.
---
Nitpick comments:
In `@simulations/vip-608/ethereum.ts`:
- Around line 28-32: Update the test "CORE_VTOKENS should cover all on-chain
markets" to compare sets instead of only lengths: fetch on-chain markets via
comptroller.getAllMarkets() into allMarkets and assert that every address in
ETHEREUM_CORE_VTOKENS appears in allMarkets and vice versa (no extras/missing)
by comparing sorted or Set representations (e.g., new Set(ETHEREUM_CORE_VTOKENS)
vs new Set(allMarkets)); keep existing variables (comptroller, getAllMarkets,
ETHEREUM_CORE_VTOKENS, allMarkets) and replace the length equality assertion
with a set-equality check that fails if any address is missing or unexpected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2e019b7-ba02-4a7e-8347-6be92bd57070
📒 Files selected for processing (8)
simulations/vip-608/arbitrumone.tssimulations/vip-608/basemainnet.tssimulations/vip-608/ethereum.tssimulations/vip-608/opbnbmainnet.tssimulations/vip-608/opmainnet.tssimulations/vip-608/unichainmainnet.tssimulations/vip-608/zksyncmainnet.tsvips/vip-608/addresses/ethereum.ts
✅ Files skipped from review due to trivial changes (3)
- simulations/vip-608/opmainnet.ts
- simulations/vip-608/unichainmainnet.ts
- simulations/vip-608/basemainnet.ts
- Rename vip-608 → vip-600 (all mainnet files and simulations) - VIP-600 (bscmainnet.ts): BNB Chain core pool VBep20Delegate upgrade, 43 markets - VIP-601 (bscmainnet-2.ts): grant syncCash() ACM permissions on 7 non-BNB chains - VIP-602 (bscmainnet-3.ts): upgrade VToken beacon + call syncCash() on all non-BNB chains - Update meta titles with VIP numbers and (part1/2/3) suffixes - Update meta descriptions from Notion docs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
vips/vip-600/addresses/basemainnet.ts (1)
3-8:⚠️ Potential issue | 🟠 MajorUse
NETWORK_ADDRESSES.basemainnetfor the canonical Base targets.These literals create another source of truth for production governance addresses. Please import the ACM, timelock, comptroller, beacon, and new-implementation values from
src/networkAddresses.tsinstead of redefining them here; keepingBASEMAINNET_DST_CHAIN_IDonLzChainIdis fine.As per coding guidelines
Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES.<network>.<key>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vips/vip-600/addresses/basemainnet.ts` around lines 3 - 8, Replace the hard-coded Base mainnet address constants (BASEMAINNET_ACM, BASEMAINNET_NORMAL_TIMELOCK, BASEMAINNET_CORE_COMPTROLLER, BASEMAINNET_VTOKEN_BEACON, BASEMAINNET_NEW_VTOKEN_IMPLEMENTATION) by importing NETWORK_ADDRESSES and assigning each from NETWORK_ADDRESSES.basemainnet (e.g., NETWORK_ADDRESSES.basemainnet.acm, .timelock, .comptroller, .vTokenBeacon, .newVTokenImplementation or the exact keys used in src/networkAddresses.ts), remove the duplicated literal strings, keep BASEMAINNET_DST_CHAIN_ID as LzChainId.basemainnet, and ensure the import for NETWORK_ADDRESSES is added at the top of the module.vips/vip-600/addresses/opmainnet.ts (1)
3-8:⚠️ Potential issue | 🟠 MajorUse
NETWORK_ADDRESSES.opmainnetfor the canonical OP Mainnet targets.These literals create another source of truth for production governance addresses. Please import the ACM, timelock, comptroller, beacon, and new-implementation values from
src/networkAddresses.tsinstead of redefining them here; keepingOPMAINNET_DST_CHAIN_IDonLzChainIdis fine.As per coding guidelines
Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES.<network>.<key>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vips/vip-600/addresses/opmainnet.ts` around lines 3 - 8, Replace the hard-coded OPMAINNET_* address constants with values from NETWORK_ADDRESSES.opmainnet: import { NETWORK_ADDRESSES } from "src/networkAddresses" and set OPMAINNET_ACM = NETWORK_ADDRESSES.opmainnet.acm (or the exact key name used there), OPMAINNET_NORMAL_TIMELOCK = NETWORK_ADDRESSES.opmainnet.timelock, OPMAINNET_CORE_COMPTROLLER = NETWORK_ADDRESSES.opmainnet.comptroller, OPMAINNET_VTOKEN_BEACON = NETWORK_ADDRESSES.opmainnet.beacon, and OPMAINNET_NEW_VTOKEN_IMPLEMENTATION = NETWORK_ADDRESSES.opmainnet.newImplementation (use the exact property names in NETWORK_ADDRESSES); leave OPMAINNET_DST_CHAIN_ID referencing LzChainId.opmainnet unchanged.simulations/vip-600/bscmainnet.ts (1)
164-166:⚠️ Potential issue | 🟠 MajorAssert
accrueInterest()returns0.
accrueInterest()returns an error code, soto.not.be.revertedstill passes on non-zero failures. Compare the return value to0instead of only checking that the call did not throw.Suggested change
- await expect(vToken.callStatic.accrueInterest()).to.not.be.reverted; + expect(await vToken.callStatic.accrueInterest()).to.equal(0);Run this read-only check to confirm the ABI output and matcher site:
#!/bin/bash set -euo pipefail echo "=== accrueInterest ABI output ===" jq '.[] | select(.type=="function" and .name=="accrueInterest") | {name, outputs}' simulations/vip-600/abi/VBep20Delegate.json echo echo "=== assertion site ===" rg -n 'callStatic\.accrueInterest\(\)|not\.be\.reverted' simulations/vip-600/bscmainnet.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-600/bscmainnet.ts` around lines 164 - 166, The test currently only checks that vToken.callStatic.accrueInterest() does not revert; change it to assert the returned error code equals 0 instead. Replace the non-revert matcher in the "should allow accrueInterest to succeed" test with a value comparison: call vToken.callStatic.accrueInterest(), await the result, and assert it equals 0 (i.e., expect(await vToken.callStatic.accrueInterest()).to.equal(0)) so you verify the function succeeded semantically rather than just not throwing.
🧹 Nitpick comments (7)
simulations/vip-600/opmainnet.ts (1)
15-15: Formatting issue: Split imports onto separate lines.Two import statements are concatenated on a single line.
🔧 Proposed fix
-import COMPTROLLER_ABI from "./abi/ILComptroller.json";import VTOKEN_ABI from "./abi/ILVToken.json"; +import COMPTROLLER_ABI from "./abi/ILComptroller.json"; +import VTOKEN_ABI from "./abi/ILVToken.json";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-600/opmainnet.ts` at line 15, Split the concatenated import statement into two separate import statements so each ABI is imported on its own line; locate the line that currently contains both COMPTROLLER_ABI and VTOKEN_ABI and change it to two distinct imports for COMPTROLLER_ABI from "./abi/ILComptroller.json" and VTOKEN_ABI from "./abi/ILVToken.json".simulations/vip-600/arbitrumone.ts (1)
15-15: Formatting issue: Split imports onto separate lines.Two import statements are concatenated on a single line, violating Prettier formatting standards.
🔧 Proposed fix
-import COMPTROLLER_ABI from "./abi/ILComptroller.json";import VTOKEN_ABI from "./abi/ILVToken.json"; +import COMPTROLLER_ABI from "./abi/ILComptroller.json"; +import VTOKEN_ABI from "./abi/ILVToken.json";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-600/arbitrumone.ts` at line 15, The two ABI imports are concatenated on one line; split them into two separate import statements so each identifier (COMPTROLLER_ABI and VTOKEN_ABI) is on its own import line (i.e., create separate import lines for "./abi/ILComptroller.json" and "./abi/ILVToken.json") and ensure proper semicolons/formatting so Prettier passes (then run formatter or lint).simulations/vip-600/zksyncmainnet.ts (1)
15-15: Formatting issue: Split imports onto separate lines.Two import statements are concatenated on a single line.
🔧 Proposed fix
-import COMPTROLLER_ABI from "./abi/ILComptroller.json";import VTOKEN_ABI from "./abi/ILVToken.json"; +import COMPTROLLER_ABI from "./abi/ILComptroller.json"; +import VTOKEN_ABI from "./abi/ILVToken.json";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-600/zksyncmainnet.ts` at line 15, The two JSON ABI imports are concatenated on one line; split them into two separate import statements so each import is on its own line (use import COMPTROLLER_ABI from "./abi/ILComptroller.json"; and import VTOKEN_ABI from "./abi/ILVToken.json";), keeping the same identifiers COMPTROLLER_ABI and VTOKEN_ABI and terminating with semicolons to match project formatting.simulations/vip-600/basemainnet.ts (1)
15-15: Formatting issue: Split imports onto separate lines.Two import statements are concatenated on a single line.
🔧 Proposed fix
-import COMPTROLLER_ABI from "./abi/ILComptroller.json";import VTOKEN_ABI from "./abi/ILVToken.json"; +import COMPTROLLER_ABI from "./abi/ILComptroller.json"; +import VTOKEN_ABI from "./abi/ILVToken.json";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-600/basemainnet.ts` at line 15, The two JSON imports are concatenated on one line causing a formatting error; split them into separate import statements so COMPTROLLER_ABI and VTOKEN_ABI are each imported on their own line (i.e., separate the combined import into two lines importing COMPTROLLER_ABI from "./abi/ILComptroller.json" and VTOKEN_ABI from "./abi/ILVToken.json"), then run the formatter/linters to confirm the file compiles and passes styling checks.simulations/vip-600/ethereum.ts (1)
15-16: Formatting issue: Two imports on the same line.Same issue as in other simulation files - line 15 has concatenated imports.
✨ Proposed fix
-import COMPTROLLER_ABI from "./abi/ILComptroller.json";import VTOKEN_ABI from "./abi/ILVToken.json"; +import COMPTROLLER_ABI from "./abi/ILComptroller.json"; +import VTOKEN_ABI from "./abi/ILVToken.json";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-600/ethereum.ts` around lines 15 - 16, The two concatenated import statements should be split so each default import is on its own line; update the import of COMPTROLLER_ABI, VTOKEN_ABI, and VTOKEN_BEACON_ABI in ethereum.ts so they are three separate import statements (import COMPTROLLER_ABI from "./abi/ILComptroller.json"; import VTOKEN_ABI from "./abi/ILVToken.json"; import VTOKEN_BEACON_ABI from "./abi/vtokenBeacon.json";) and run the formatter/linter to ensure consistent style.simulations/vip-600/unichainmainnet.ts (1)
15-16: Formatting issue: Two imports on the same line.Line 15 has two import statements concatenated without a line break, which violates formatting standards.
✨ Proposed fix
-import COMPTROLLER_ABI from "./abi/ILComptroller.json";import VTOKEN_ABI from "./abi/ILVToken.json"; +import COMPTROLLER_ABI from "./abi/ILComptroller.json"; +import VTOKEN_ABI from "./abi/ILVToken.json";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-600/unichainmainnet.ts` around lines 15 - 16, The import line combining two statements should be split into separate import declarations: ensure COMPTROLLER_ABI and VTOKEN_ABI are each imported on their own line (keep the existing VTOKEN_BEACON_ABI import as-is); update the line containing "import COMPTROLLER_ABI from \"./abi/ILComptroller.json\";import VTOKEN_ABI from \"./abi/ILVToken.json\";" so there is a newline between the two import statements and consistent semicolon/formatting, referencing the symbols COMPTROLLER_ABI, VTOKEN_ABI, and VTOKEN_BEACON_ABI to locate the change.simulations/vip-600/opbnbmainnet.ts (1)
15-16: Formatting issue: Two imports on the same line.Same issue as in other simulation files - line 15 has concatenated imports.
✨ Proposed fix
-import COMPTROLLER_ABI from "./abi/ILComptroller.json";import VTOKEN_ABI from "./abi/ILVToken.json"; +import COMPTROLLER_ABI from "./abi/ILComptroller.json"; +import VTOKEN_ABI from "./abi/ILVToken.json";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-600/opbnbmainnet.ts` around lines 15 - 16, The import statement currently combines two imports on one line; split the concatenated import so each module is imported on its own line: create separate import statements for COMPTROLLER_ABI and VTOKEN_ABI (the existing VTOKEN_BEACON_ABI import can remain unchanged) to match the formatting used across other simulation files and avoid syntax/formatting issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vips/vip-600/addresses/arbitrumone.ts`:
- Around line 3-8: These constants (ARBITRUMONE_ACM,
ARBITRUMONE_NORMAL_TIMELOCK, ARBITRUMONE_CORE_COMPTROLLER,
ARBITRUMONE_VTOKEN_BEACON, ARBITRUMONE_NEW_VTOKEN_IMPLEMENTATION,
ARBITRUMONE_DST_CHAIN_ID) should be sourced from the canonical NETWORK_ADDRESSES
object instead of hardcoded strings; update the module to import
NETWORK_ADDRESSES from src/networkAddresses.ts and replace each export with the
corresponding NETWORK_ADDRESSES.arbitrumone.<key> entry (e.g.,
NETWORK_ADDRESSES.arbitrumone.acm, .normalTimelock, .coreComptroller,
.vTokenBeacon, .newVTokenImplementation, .dstChainId) so all Arbitrum targets
reference the single source of truth.
In `@vips/vip-600/addresses/ethereum.ts`:
- Around line 3-8: Replace the hard-coded constants ETHEREUM_ACM,
ETHEREUM_NORMAL_TIMELOCK, ETHEREUM_CORE_COMPTROLLER, ETHEREUM_VTOKEN_BEACON and
ETHEREUM_NEW_VTOKEN_IMPLEMENTATION with values from the shared registry via
NETWORK_ADDRESSES.ethereum.<key>; add an import for NETWORK_ADDRESSES and use
NETWORK_ADDRESSES.ethereum.acm / .timelock / .comptroller / .vtokenBeacon /
.newImplementation (or the exact keys used in the registry) instead of the
string literals, leaving ETHEREUM_DST_CHAIN_ID (LzChainId.ethereum) intact and
remove the now-unused literal exports.
In `@vips/vip-600/addresses/opbnbmainnet.ts`:
- Around line 3-8: The constants OPBNBMAINNET_ACM, OPBNBMAINNET_NORMAL_TIMELOCK,
OPBNBMAINNET_CORE_COMPTROLLER, OPBNBMAINNET_VTOKEN_BEACON,
OPBNBMAINNET_NEW_VTOKEN_IMPLEMENTATION and OPBNBMAINNET_DST_CHAIN_ID are
file-local literals and must be replaced by values from NETWORK_ADDRESSES;
import NETWORK_ADDRESSES from src/networkAddresses.ts and replace each constant
with the corresponding NETWORK_ADDRESSES.opbnbmainnet.<key> (e.g.,
NETWORK_ADDRESSES.opbnbmainnet.acm, .timelock, .comptroller, .vtokenBeacon,
.vtokenImplementation) and for DST_CHAIN_ID use
NETWORK_ADDRESSES.opbnbmainnet.dstChainId or the provided LZ mapping if present,
removing the duplicated hardcoded strings. Ensure the export names
(OPBNBMAINNET_*) remain the same but reference the imported NETWORK_ADDRESSES
entries.
In `@vips/vip-600/addresses/zksyncmainnet.ts`:
- Around line 3-8: Replace the hardcoded address constants in zksyncmainnet.ts
(ZKSYNCMAINNET_ACM, ZKSYNCMAINNET_NORMAL_TIMELOCK,
ZKSYNCMAINNET_CORE_COMPTROLLER, ZKSYNCMAINNET_VTOKEN_BEACON,
ZKSYNCMAINNET_NEW_VTOKEN_IMPLEMENTATION, and ZKSYNCMAINNET_DST_CHAIN_ID) to read
from the canonical NETWORK_ADDRESSES export in src/networkAddresses.ts (e.g.
NETWORK_ADDRESSES.zksyncmainnet.<key>) and import NETWORK_ADDRESSES at the top
of the file; ensure each constant maps to the corresponding key name used in
networkAddresses.ts and keep DST_CHAIN_ID using LzChainId if that mapping is
provided by NETWORK_ADDRESSES.
In `@vips/vip-600/bscmainnet.ts`:
- Around line 120-132: The proposal text claims excess donated tokens (e.g.,
vTHE) will be recovered via proxy.sweepTokenAndSync(uint256 transferAmount), but
every call to sweepTokenAndSync in MARKETS is invoked with 0 so no tokens are
transferred; either populate MARKETS (or the upgrade step) with the verified
non-zero excessAmount for affected markets (e.g., vTHE) so
proxy.sweepTokenAndSync(excessAmount) will transfer the donation to Timelock and
initialise internalCash, or update the narrative to state the VIP only
initialises internalCash (no token recovery). Locate uses of sweepTokenAndSync,
proxy, MARKETS and internalCash in the diff and implement one of these two fixes
consistently across all occurrences.
- Around line 4-50: Replace the hardcoded addresses in this file
(NEW_VBEP20_DELEGATE_IMPL and all VBep20Delegator constants like vUSDC, vUSDT,
vBUSD, ..., vXAUM) with imports from the canonical address registry: import
NETWORK_ADDRESSES from src/networkAddresses.ts and reference
NETWORK_ADDRESSES.bsc.<key> (use the matching keys for the new implementation
and each market) so this VIP no longer contains literal production addresses and
instead uses NETWORK_ADDRESSES.<network>.<key>.
---
Duplicate comments:
In `@simulations/vip-600/bscmainnet.ts`:
- Around line 164-166: The test currently only checks that
vToken.callStatic.accrueInterest() does not revert; change it to assert the
returned error code equals 0 instead. Replace the non-revert matcher in the
"should allow accrueInterest to succeed" test with a value comparison: call
vToken.callStatic.accrueInterest(), await the result, and assert it equals 0
(i.e., expect(await vToken.callStatic.accrueInterest()).to.equal(0)) so you
verify the function succeeded semantically rather than just not throwing.
In `@vips/vip-600/addresses/basemainnet.ts`:
- Around line 3-8: Replace the hard-coded Base mainnet address constants
(BASEMAINNET_ACM, BASEMAINNET_NORMAL_TIMELOCK, BASEMAINNET_CORE_COMPTROLLER,
BASEMAINNET_VTOKEN_BEACON, BASEMAINNET_NEW_VTOKEN_IMPLEMENTATION) by importing
NETWORK_ADDRESSES and assigning each from NETWORK_ADDRESSES.basemainnet (e.g.,
NETWORK_ADDRESSES.basemainnet.acm, .timelock, .comptroller, .vTokenBeacon,
.newVTokenImplementation or the exact keys used in src/networkAddresses.ts),
remove the duplicated literal strings, keep BASEMAINNET_DST_CHAIN_ID as
LzChainId.basemainnet, and ensure the import for NETWORK_ADDRESSES is added at
the top of the module.
In `@vips/vip-600/addresses/opmainnet.ts`:
- Around line 3-8: Replace the hard-coded OPMAINNET_* address constants with
values from NETWORK_ADDRESSES.opmainnet: import { NETWORK_ADDRESSES } from
"src/networkAddresses" and set OPMAINNET_ACM = NETWORK_ADDRESSES.opmainnet.acm
(or the exact key name used there), OPMAINNET_NORMAL_TIMELOCK =
NETWORK_ADDRESSES.opmainnet.timelock, OPMAINNET_CORE_COMPTROLLER =
NETWORK_ADDRESSES.opmainnet.comptroller, OPMAINNET_VTOKEN_BEACON =
NETWORK_ADDRESSES.opmainnet.beacon, and OPMAINNET_NEW_VTOKEN_IMPLEMENTATION =
NETWORK_ADDRESSES.opmainnet.newImplementation (use the exact property names in
NETWORK_ADDRESSES); leave OPMAINNET_DST_CHAIN_ID referencing LzChainId.opmainnet
unchanged.
---
Nitpick comments:
In `@simulations/vip-600/arbitrumone.ts`:
- Line 15: The two ABI imports are concatenated on one line; split them into two
separate import statements so each identifier (COMPTROLLER_ABI and VTOKEN_ABI)
is on its own import line (i.e., create separate import lines for
"./abi/ILComptroller.json" and "./abi/ILVToken.json") and ensure proper
semicolons/formatting so Prettier passes (then run formatter or lint).
In `@simulations/vip-600/basemainnet.ts`:
- Line 15: The two JSON imports are concatenated on one line causing a
formatting error; split them into separate import statements so COMPTROLLER_ABI
and VTOKEN_ABI are each imported on their own line (i.e., separate the combined
import into two lines importing COMPTROLLER_ABI from "./abi/ILComptroller.json"
and VTOKEN_ABI from "./abi/ILVToken.json"), then run the formatter/linters to
confirm the file compiles and passes styling checks.
In `@simulations/vip-600/ethereum.ts`:
- Around line 15-16: The two concatenated import statements should be split so
each default import is on its own line; update the import of COMPTROLLER_ABI,
VTOKEN_ABI, and VTOKEN_BEACON_ABI in ethereum.ts so they are three separate
import statements (import COMPTROLLER_ABI from "./abi/ILComptroller.json";
import VTOKEN_ABI from "./abi/ILVToken.json"; import VTOKEN_BEACON_ABI from
"./abi/vtokenBeacon.json";) and run the formatter/linter to ensure consistent
style.
In `@simulations/vip-600/opbnbmainnet.ts`:
- Around line 15-16: The import statement currently combines two imports on one
line; split the concatenated import so each module is imported on its own line:
create separate import statements for COMPTROLLER_ABI and VTOKEN_ABI (the
existing VTOKEN_BEACON_ABI import can remain unchanged) to match the formatting
used across other simulation files and avoid syntax/formatting issues.
In `@simulations/vip-600/opmainnet.ts`:
- Line 15: Split the concatenated import statement into two separate import
statements so each ABI is imported on its own line; locate the line that
currently contains both COMPTROLLER_ABI and VTOKEN_ABI and change it to two
distinct imports for COMPTROLLER_ABI from "./abi/ILComptroller.json" and
VTOKEN_ABI from "./abi/ILVToken.json".
In `@simulations/vip-600/unichainmainnet.ts`:
- Around line 15-16: The import line combining two statements should be split
into separate import declarations: ensure COMPTROLLER_ABI and VTOKEN_ABI are
each imported on their own line (keep the existing VTOKEN_BEACON_ABI import
as-is); update the line containing "import COMPTROLLER_ABI from
\"./abi/ILComptroller.json\";import VTOKEN_ABI from \"./abi/ILVToken.json\";" so
there is a newline between the two import statements and consistent
semicolon/formatting, referencing the symbols COMPTROLLER_ABI, VTOKEN_ABI, and
VTOKEN_BEACON_ABI to locate the change.
In `@simulations/vip-600/zksyncmainnet.ts`:
- Line 15: The two JSON ABI imports are concatenated on one line; split them
into two separate import statements so each import is on its own line (use
import COMPTROLLER_ABI from "./abi/ILComptroller.json"; and import VTOKEN_ABI
from "./abi/ILVToken.json";), keeping the same identifiers COMPTROLLER_ABI and
VTOKEN_ABI and terminating with semicolons to match project formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e800063-95a1-4ff4-be73-46d97a603397
📒 Files selected for processing (25)
simulations/vip-600/abi/ILComptroller.jsonsimulations/vip-600/abi/ILPoolRegistry.jsonsimulations/vip-600/abi/ILVToken.jsonsimulations/vip-600/abi/VBep20Delegate.jsonsimulations/vip-600/abi/vtokenBeacon.jsonsimulations/vip-600/arbitrumone.tssimulations/vip-600/basemainnet.tssimulations/vip-600/bscmainnet-2.tssimulations/vip-600/bscmainnet-3.tssimulations/vip-600/bscmainnet.tssimulations/vip-600/ethereum.tssimulations/vip-600/opbnbmainnet.tssimulations/vip-600/opmainnet.tssimulations/vip-600/unichainmainnet.tssimulations/vip-600/zksyncmainnet.tsvips/vip-600/addresses/arbitrumone.tsvips/vip-600/addresses/basemainnet.tsvips/vip-600/addresses/ethereum.tsvips/vip-600/addresses/opbnbmainnet.tsvips/vip-600/addresses/opmainnet.tsvips/vip-600/addresses/unichainmainnet.tsvips/vip-600/addresses/zksyncmainnet.tsvips/vip-600/bscmainnet-2.tsvips/vip-600/bscmainnet-3.tsvips/vip-600/bscmainnet.ts
✅ Files skipped from review due to trivial changes (5)
- simulations/vip-600/abi/vtokenBeacon.json
- simulations/vip-600/abi/VBep20Delegate.json
- simulations/vip-600/abi/ILComptroller.json
- simulations/vip-600/abi/ILVToken.json
- simulations/vip-600/abi/ILPoolRegistry.json
Comment on lines
+3
to
+8
| export const ARBITRUMONE_ACM = "0xD9dD18EB0cf10CbA837677f28A8F9Bda4bc2b157"; | ||
| export const ARBITRUMONE_NORMAL_TIMELOCK = "0x4b94589Cc23F618687790036726f744D602c4017"; | ||
| export const ARBITRUMONE_CORE_COMPTROLLER = "0x317c1A5739F39046E20b08ac9BeEa3f10fD43326"; | ||
| export const ARBITRUMONE_VTOKEN_BEACON = "0xE9381D8CA7006c12Ae9eB97890575E705996fa66"; | ||
| export const ARBITRUMONE_NEW_VTOKEN_IMPLEMENTATION = "0x1986Fb53535953711265d5fD329cd7A690411669"; | ||
| export const ARBITRUMONE_DST_CHAIN_ID = LzChainId.arbitrumone; |
There was a problem hiding this comment.
Use NETWORK_ADDRESSES.arbitrumone for the canonical Arbitrum targets.
These governance addresses should come from src/networkAddresses.ts instead of being embedded again in this module.
As per coding guidelines Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES.<network>.<key>.
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 7-7: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vips/vip-600/addresses/arbitrumone.ts` around lines 3 - 8, These constants
(ARBITRUMONE_ACM, ARBITRUMONE_NORMAL_TIMELOCK, ARBITRUMONE_CORE_COMPTROLLER,
ARBITRUMONE_VTOKEN_BEACON, ARBITRUMONE_NEW_VTOKEN_IMPLEMENTATION,
ARBITRUMONE_DST_CHAIN_ID) should be sourced from the canonical NETWORK_ADDRESSES
object instead of hardcoded strings; update the module to import
NETWORK_ADDRESSES from src/networkAddresses.ts and replace each export with the
corresponding NETWORK_ADDRESSES.arbitrumone.<key> entry (e.g.,
NETWORK_ADDRESSES.arbitrumone.acm, .normalTimelock, .coreComptroller,
.vTokenBeacon, .newVTokenImplementation, .dstChainId) so all Arbitrum targets
reference the single source of truth.
Comment on lines
+3
to
+8
| export const ETHEREUM_ACM = "0x230058da2D23eb8836EC5DB7037ef7250c56E25E"; | ||
| export const ETHEREUM_NORMAL_TIMELOCK = "0xd969E79406c35E80750aAae061D402Aab9325714"; | ||
| export const ETHEREUM_CORE_COMPTROLLER = "0x687a01ecF6d3907658f7A7c714749fAC32336D1B"; | ||
| export const ETHEREUM_VTOKEN_BEACON = "0xfc08aADC7a1A93857f6296C3fb78aBA1d286533a"; | ||
| export const ETHEREUM_NEW_VTOKEN_IMPLEMENTATION = "0x33bE30B31f07c8a2bfb705FBcE55E983c47ba864"; | ||
| export const ETHEREUM_DST_CHAIN_ID = LzChainId.ethereum; |
There was a problem hiding this comment.
Use NETWORK_ADDRESSES.ethereum for the canonical Ethereum targets.
These address literals should come from the shared network registry rather than a file-local copy. Please import the ACM, timelock, comptroller, beacon, and new-implementation values from src/networkAddresses.ts.
As per coding guidelines Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES.<network>.<key>.
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 7-7: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vips/vip-600/addresses/ethereum.ts` around lines 3 - 8, Replace the
hard-coded constants ETHEREUM_ACM, ETHEREUM_NORMAL_TIMELOCK,
ETHEREUM_CORE_COMPTROLLER, ETHEREUM_VTOKEN_BEACON and
ETHEREUM_NEW_VTOKEN_IMPLEMENTATION with values from the shared registry via
NETWORK_ADDRESSES.ethereum.<key>; add an import for NETWORK_ADDRESSES and use
NETWORK_ADDRESSES.ethereum.acm / .timelock / .comptroller / .vtokenBeacon /
.newImplementation (or the exact keys used in the registry) instead of the
string literals, leaving ETHEREUM_DST_CHAIN_ID (LzChainId.ethereum) intact and
remove the now-unused literal exports.
Comment on lines
+3
to
+8
| export const OPBNBMAINNET_ACM = "0xA60Deae5344F1152426cA440fb6552eA0e3005D6"; | ||
| export const OPBNBMAINNET_NORMAL_TIMELOCK = "0x10f504e939b912569Dca611851fDAC9E3Ef86819"; | ||
| export const OPBNBMAINNET_CORE_COMPTROLLER = "0xD6e3E2A1d8d95caE355D15b3b9f8E5c2511874dd"; | ||
| export const OPBNBMAINNET_VTOKEN_BEACON = "0xfeD1d3a13597c5aBc893Af41ED5cb17e64c847c7"; | ||
| export const OPBNBMAINNET_NEW_VTOKEN_IMPLEMENTATION = "0x7AA7EB7553DE5f221C06595f3b4021363279e9Fe"; | ||
| export const OPBNBMAINNET_DST_CHAIN_ID = LzChainId.opbnbmainnet; |
There was a problem hiding this comment.
Use NETWORK_ADDRESSES.opbnbmainnet instead of file-local literals.
The ACM, timelock, comptroller, beacon, and implementation addresses should be sourced from src/networkAddresses.ts, not duplicated here.
As per coding guidelines Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES.<network>.<key>.
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 7-7: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vips/vip-600/addresses/opbnbmainnet.ts` around lines 3 - 8, The constants
OPBNBMAINNET_ACM, OPBNBMAINNET_NORMAL_TIMELOCK, OPBNBMAINNET_CORE_COMPTROLLER,
OPBNBMAINNET_VTOKEN_BEACON, OPBNBMAINNET_NEW_VTOKEN_IMPLEMENTATION and
OPBNBMAINNET_DST_CHAIN_ID are file-local literals and must be replaced by values
from NETWORK_ADDRESSES; import NETWORK_ADDRESSES from src/networkAddresses.ts
and replace each constant with the corresponding
NETWORK_ADDRESSES.opbnbmainnet.<key> (e.g., NETWORK_ADDRESSES.opbnbmainnet.acm,
.timelock, .comptroller, .vtokenBeacon, .vtokenImplementation) and for
DST_CHAIN_ID use NETWORK_ADDRESSES.opbnbmainnet.dstChainId or the provided LZ
mapping if present, removing the duplicated hardcoded strings. Ensure the export
names (OPBNBMAINNET_*) remain the same but reference the imported
NETWORK_ADDRESSES entries.
Comment on lines
+3
to
+8
| export const ZKSYNCMAINNET_ACM = "0x526159A92A82afE5327d37Ef446b68FD9a5cA914"; | ||
| export const ZKSYNCMAINNET_NORMAL_TIMELOCK = "0x093565Bc20AA326F4209eBaF3a26089272627613"; | ||
| export const ZKSYNCMAINNET_CORE_COMPTROLLER = "0xddE4D098D9995B659724ae6d5E3FB9681Ac941B1"; | ||
| export const ZKSYNCMAINNET_VTOKEN_BEACON = "0x53523537aa330640B80400EB8B309fF5896E7eb5"; | ||
| export const ZKSYNCMAINNET_NEW_VTOKEN_IMPLEMENTATION = "0x3e9cf1eBe018610585D0B519A9956C950E079591"; | ||
| export const ZKSYNCMAINNET_DST_CHAIN_ID = LzChainId.zksyncmainnet; |
There was a problem hiding this comment.
Use NETWORK_ADDRESSES.zksyncmainnet instead of embedded literals.
Hardcoding the ACM, timelock, comptroller, beacon, and implementation addresses here bypasses the repo’s canonical network registry. Please source those values from src/networkAddresses.ts.
As per coding guidelines Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES.<network>.<key>.
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 7-7: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vips/vip-600/addresses/zksyncmainnet.ts` around lines 3 - 8, Replace the
hardcoded address constants in zksyncmainnet.ts (ZKSYNCMAINNET_ACM,
ZKSYNCMAINNET_NORMAL_TIMELOCK, ZKSYNCMAINNET_CORE_COMPTROLLER,
ZKSYNCMAINNET_VTOKEN_BEACON, ZKSYNCMAINNET_NEW_VTOKEN_IMPLEMENTATION, and
ZKSYNCMAINNET_DST_CHAIN_ID) to read from the canonical NETWORK_ADDRESSES export
in src/networkAddresses.ts (e.g. NETWORK_ADDRESSES.zksyncmainnet.<key>) and
import NETWORK_ADDRESSES at the top of the file; ensure each constant maps to
the corresponding key name used in networkAddresses.ts and keep DST_CHAIN_ID
using LzChainId if that mapping is provided by NETWORK_ADDRESSES.
Comment on lines
+4
to
+50
| const NEW_VBEP20_DELEGATE_IMPL = "0xb25b57599BA969c4829699F7E4Fc4076D14745E1"; | ||
|
|
||
| // VBep20Delegator proxy addresses (43 markets, excludes vBNB) | ||
| const vUSDC = "0xecA88125a5ADbe82614ffC12D0DB554E2e2867C8"; | ||
| const vUSDT = "0xfD5840Cd36d94D7229439859C0112a4185BC0255"; | ||
| const vBUSD = "0x95c78222B3D6e262426483D42CfA53685A67Ab9D"; | ||
| const vSXP = "0x2fF3d0F6990a40261c66E1ff2017aCBc282EB6d0"; | ||
| const vXVS = "0x151B1e2635A717bcDc836ECd6FbB62B674FE3E1D"; | ||
| const vBTC = "0x882C173bC7Ff3b7786CA16dfeD3DFFfb9Ee7847B"; | ||
| const vETH = "0xf508fCD89b8bd15579dc79A6827cB4686A3592c8"; | ||
| const vLTC = "0x57A5297F2cB2c0AaC9D554660acd6D385Ab50c6B"; | ||
| const vXRP = "0xB248a295732e0225acd3337607cc01068e3b9c10"; | ||
| const vBCH = "0x5F0388EBc2B94FA8E123F404b79cCF5f40b29176"; | ||
| const vDOT = "0x1610bc33319e9398de5f57B33a5b184c806aD217"; | ||
| const vLINK = "0x650b940a1033B8A1b1873f78730FcFC73ec11f1f"; | ||
| const vDAI = "0x334b3eCB4DCa3593BCCC3c7EBD1A1C1d1780FBF1"; | ||
| const vFIL = "0xf91d58b5aE142DAcC749f58A49FCBac340Cb0343"; | ||
| const vBETH = "0x972207A639CC1B374B893cc33Fa251b55CEB7c07"; | ||
| const vADA = "0x9A0AF7FDb2065Ce470D72664DE73cAE409dA28Ec"; | ||
| const vDOGE = "0xec3422Ef92B2fb59e84c8B02Ba73F1fE84Ed8D71"; | ||
| const vMATIC = "0x5c9476FcD6a4F9a3654139721c949c2233bBbBc8"; | ||
| const vCAKE = "0x86aC3974e2BD0d60825230fa6F355fF11409df5c"; | ||
| const vAAVE = "0x26DA28954763B92139ED49283625ceCAf52C6f94"; | ||
| const vTUSDOLD = "0x08CEB3F4a7ed3500cA0982bcd0FC7816688084c3"; | ||
| const vTRXOLD = "0x61eDcFe8Dd6bA3c891CB9bEc2dc7657B3B422E93"; | ||
| const vTRX = "0xC5D3466aA484B040eE977073fcF337f2c00071c1"; | ||
| const vWBETH = "0x6CFdEc747f37DAf3b87a35a1D9c8AD3063A1A8A0"; | ||
| const vTUSD = "0xBf762cd5991cA1DCdDaC9ae5C638F5B5Dc3Bee6E"; | ||
| const vUNI = "0x27FF564707786720C71A2e5c1490A63266683612"; | ||
| const vFDUSD = "0xC4eF4229FEc74Ccfe17B2bdeF7715fAC740BA0ba"; | ||
| const vTWT = "0x4d41a36D04D97785bcEA57b057C412b278e6Edcc"; | ||
| const vSolvBTC = "0xf841cb62c19fCd4fF5CD0AaB5939f3140BaaC3Ea"; | ||
| const vTHE = "0x86e06EAfa6A1eA631Eab51DE500E3D474933739f"; | ||
| const vSOL = "0xBf515bA4D1b52FFdCeaBF20d31D705Ce789F2cEC"; | ||
| const vlisUSD = "0x689E0daB47Ab16bcae87Ec18491692BF621Dc6Ab"; | ||
| const vPT_sUSDE_26JUN2025 = "0x9e4E5fed5Ac5B9F732d0D850A615206330Bf1866"; | ||
| const vsUSDe = "0x699658323d58eE25c69F1a29d476946ab011bD18"; | ||
| const vUSDe = "0x74ca6930108F775CC667894EEa33843e691680d7"; | ||
| const vUSD1 = "0x0C1DA220D301155b87318B90692Da8dc43B67340"; | ||
| const vxSolvBTC = "0xd804dE60aFD05EE6B89aab5D152258fD461B07D5"; | ||
| const vasBNB = "0xCC1dB43a06d97f736C7B045AedD03C6707c09BDF"; | ||
| const vWBNB = "0x6bCa74586218dB34cdB402295796b79663d816e9"; | ||
| const vslisBNB = "0x89c910Eb8c90df818b4649b508Ba22130Dc73Adc"; | ||
| const vU = "0x3d5E269787d562b74aCC55F18Bd26C5D09Fa245E"; | ||
| const vPT_clisBNB_25JUN2026 = "0x6d3BD68E90B42615cb5abF4B8DE92b154ADc435e"; | ||
| const vXAUM = "0x92e6Ea74a1A3047DabF4186405a21c7D63a0612A"; | ||
|
|
There was a problem hiding this comment.
Move the BSC implementation and market targets to the canonical address source.
This VIP hardcodes the new implementation plus all 43 market addresses inline, which makes the proposal another source of truth for production targets. Please source them from src/networkAddresses.ts (or a shared registry derived from it) instead of embedding literals here.
As per coding guidelines Network addresses must be imported from src/networkAddresses.ts via NETWORK_ADDRESSES.<network>.<key>.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vips/vip-600/bscmainnet.ts` around lines 4 - 50, Replace the hardcoded
addresses in this file (NEW_VBEP20_DELEGATE_IMPL and all VBep20Delegator
constants like vUSDC, vUSDT, vBUSD, ..., vXAUM) with imports from the canonical
address registry: import NETWORK_ADDRESSES from src/networkAddresses.ts and
reference NETWORK_ADDRESSES.bsc.<key> (use the matching keys for the new
implementation and each market) so this VIP no longer contains literal
production addresses and instead uses NETWORK_ADDRESSES.<network>.<key>.
Comment on lines
+120
to
+132
| 2. **Call sweepTokenAndSync(excessAmount) on each upgraded market** | ||
| - Function: proxy.sweepTokenAndSync(uint256 transferAmount) | ||
| - Must be executed atomically in the same VIP as the implementation upgrade | ||
| - Pass 0 for markets with no excess tokens; pass the excess amount for markets with prior donations (e.g. vTHE) to transfer excess to the Timelock before syncing | ||
| - Initialises internalCash to match balanceOf(address(this)) post-upgrade | ||
|
|
||
| #### Summary | ||
|
|
||
| If approved, this VIP will: | ||
| - Upgrade the VBep20Delegate implementation for all 43 BNB Chain core pool VBep20 markets to use internal cash tracking | ||
| - Close the donation attack vector by making getCashPrior() immune to direct token transfers | ||
| - Initialise internalCash on all upgraded markets via sweepTokenAndSync() in the same transaction | ||
| - Recover any excess donated tokens (including from the vTHE exploit) to the Timelock |
There was a problem hiding this comment.
The description says tokens will be swept, but the commands only sync cash.
Lines 120-132 promise recovery of excess donations, including the vTHE exploit balance, yet every sweepTokenAndSync call at Lines 155-158 passes 0. Either wire the real non-zero excessAmount values into MARKETS or trim the narrative to say this proposal only initializes internalCash.
Based on learnings Numeric values in VIP proposals must be independently verified before submission.
Also applies to: 149-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vips/vip-600/bscmainnet.ts` around lines 120 - 132, The proposal text claims
excess donated tokens (e.g., vTHE) will be recovered via
proxy.sweepTokenAndSync(uint256 transferAmount), but every call to
sweepTokenAndSync in MARKETS is invoked with 0 so no tokens are transferred;
either populate MARKETS (or the upgrade step) with the verified non-zero
excessAmount for affected markets (e.g., vTHE) so
proxy.sweepTokenAndSync(excessAmount) will transfer the donation to Timelock and
initialise internalCash, or update the narrative to state the VIP only
initialises internalCash (no token recovery). Locate uses of sweepTokenAndSync,
proxy, MARKETS and internalCash in the diff and implement one of these two fixes
consistently across all occurrences.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
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
2 participants
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.
VIP-600 (part1): [BNB Chain] VToken Inflation Attack Patch
Title: [BNB Chain] VToken Inflation Attack Patch (part1)
This VIP upgrades the
VBep20Delegateimplementation across all 43 BNB Chain core pool VBep20 markets to patch the donation attack vulnerability identified in the THE market incident. The root cause is ingetCashPrior()withinVBep20.sol, which previously returnedIERC20(underlying).balanceOf(address(this))— an actual on-chain balance that any external actor can inflate by transferring tokens directly to the vToken contract address.The fix replaces balance-based cash accounting with an
internalCashstorage variable that is only updated through protocol-controlled operations (doTransferInanddoTransferOut). A new admin-gatedsweepTokenAndSync()function is called atomically in the same VIP to initialiseinternalCashand recover any excess donated tokens to the Timelock.Changes
VBep20Delegateimplementation for all 43 core pool markets —proxy._setImplementation(newImpl, false, "0x")sweepTokenAndSync(excessAmount)on each upgraded market — initialisesinternalCash, sweeps excess tokens to TimelockReferences
VIP-601 (part2): [Non-BNB Chain] Grant syncCash Permissions
Title: [Non-BNB Chain] VToken Inflation Attack Patch and Pause Borrowing Across All Markets (part2)
Grants
syncCash()call permissions to the Normal Timelock on all 7 non-BNB Chain networks (Arbitrum, Base, Ethereum, opBNB, Optimism, Unichain, zkSync). Prerequisite for VIP-602.References
VIP-602 (part3): [Non-BNB Chain] Upgrade VToken Beacon + syncCash
Title: [Non-BNB Chain] VToken Inflation Attack Patch and Pause Borrowing Across All Markets (part3)
Upgrades the VToken beacon to the new
internalCash-based implementation on all 7 non-BNB Chain networks, callssyncCash()on every market to initialize internal cash accounting, and restores borrowing across all previously paused markets.Changes
upgradeTo(newImpl)syncCash()on each VToken market to initializeinternalCashReferences
Summary by CodeRabbit