[WONT MERGE]: Exclude deprecated markets from PoolLens#553
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
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:
📝 WalkthroughWalkthroughAdded an immutable Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant PoolLens as PoolLens
participant Comptroller as Comptroller
participant VToken as VToken
Caller->>PoolLens: call view (e.g., getPoolByComptroller / getPoolBadDebt)
PoolLens->>Comptroller: getAllMarkets()
alt comptroller == corePoolComptroller
Comptroller-->>PoolLens: all markets
PoolLens-->>Caller: return full markets list
else
Comptroller-->>PoolLens: all markets
loop for each market
PoolLens->>VToken: call internalCash()
VToken-->>PoolLens: internalCash value
end
PoolLens-->>Caller: return filtered markets (internalCash() > 0)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/Lens/PoolLens.sol (1)
249-263:⚠️ Potential issue | 🟠 MajorDon't reuse the active-market filter for accounting endpoints.
At Line 253 and Line 281,
getPendingRewards()andgetPoolBadDebt()now skip markets that are unlisted or fully paused. Offboarding a market does not clear existing balances, so those markets can still contribute user reward deltas and non-zerobadDebt(). That turns both methods into active-only subsets while their names still read as full pool/account summaries. I’d keep_getActiveMarkets()on metadata/listing paths and leave these aggregations on the full market set, or expose separate active-only variants if that API change is intentional.Also applies to: 276-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/Lens/PoolLens.sol` around lines 249 - 263, getPendingRewards() and getPoolBadDebt() wrongly use _getActiveMarkets(), which filters out unlisted/paused markets and therefore omits balances and rewards from offboarded markets; replace the active-market filter with the full market set when computing aggregations by calling the comptroller's full-market accessor (e.g., ComptrollerInterface(comptrollerAddress).getAllMarkets() or the equivalent getAllMarkets() on ComptrollerViewInterface) and use that result instead of _getActiveMarkets(); keep _getActiveMarkets() for listing/metadata endpoints only or add separate active-only variants if intended.
🧹 Nitpick comments (1)
tests/hardhat/Fork/PoolLensForkTest.ts (1)
78-89: Assert the full active-market predicate in these cases.At Line 86 and Line 147, these checks only verify
isListed. A market that is still listed but has both MINT and BORROW paused would still pass here, so these tests do not validate the exact_isActiveMarketbehavior added by the contract forgetAllPools()andgetPoolDataFromVenusPool(). ReuseisActiveMarket()for each returned vToken.Also applies to: 139-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/PoolLensForkTest.ts` around lines 78 - 89, The tests calling poolLens.getAllPools and getPoolDataFromVenusPool currently only assert vToken.isListed but must assert the full active-market predicate; update the assertions to call and assert the same logic as the contract's isActiveMarket (i.e., replace or augment checks that vToken.isListed in the loops with a call to the helper/isActiveMarket predicate used by the contract or reimplement that predicate in the test and assert it returns true for each returned vToken), ensuring both MINT and BORROW pause flags and listed status are validated for each vToken returned by getAllPools and getPoolDataFromVenusPool.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/hardhat/Fork/PoolLensForkTest.ts`:
- Around line 15-18: The module currently destructures POOL_REGISTRY,
COMPTROLLER, ACC1, BLOCK_NUMBER from getContractAddresses(FORKED_NETWORK) at
load time which will throw if getContractAddresses returns undefined for
unsupported names; change to first validate FORKED_NETWORK (e.g., check against
the known network list or ensure getContractAddresses(FORKED_NETWORK) !==
undefined) or delay calling getContractAddresses until inside the FORK guard, by
assigning its return to a temporary (e.g., addresses) and only destructuring
POOL_REGISTRY, COMPTROLLER, ACC1, BLOCK_NUMBER after confirming addresses is
defined; ensure the same pattern is applied to the other destructurings
mentioned around lines 21–30.
---
Outside diff comments:
In `@contracts/Lens/PoolLens.sol`:
- Around line 249-263: getPendingRewards() and getPoolBadDebt() wrongly use
_getActiveMarkets(), which filters out unlisted/paused markets and therefore
omits balances and rewards from offboarded markets; replace the active-market
filter with the full market set when computing aggregations by calling the
comptroller's full-market accessor (e.g.,
ComptrollerInterface(comptrollerAddress).getAllMarkets() or the equivalent
getAllMarkets() on ComptrollerViewInterface) and use that result instead of
_getActiveMarkets(); keep _getActiveMarkets() for listing/metadata endpoints
only or add separate active-only variants if intended.
---
Nitpick comments:
In `@tests/hardhat/Fork/PoolLensForkTest.ts`:
- Around line 78-89: The tests calling poolLens.getAllPools and
getPoolDataFromVenusPool currently only assert vToken.isListed but must assert
the full active-market predicate; update the assertions to call and assert the
same logic as the contract's isActiveMarket (i.e., replace or augment checks
that vToken.isListed in the loops with a call to the helper/isActiveMarket
predicate used by the contract or reimplement that predicate in the test and
assert it returns true for each returned vToken), ensuring both MINT and BORROW
pause flags and listed status are validated for each vToken returned by
getAllPools and getPoolDataFromVenusPool.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a27bb62b-36e6-4dcc-8659-b11549750158
📒 Files selected for processing (2)
contracts/Lens/PoolLens.soltests/hardhat/Fork/PoolLensForkTest.ts
| const FORK = process.env.FORK === "true"; | ||
| const FORKED_NETWORK = process.env.FORKED_NETWORK || "bscmainnet"; | ||
|
|
||
| const { POOL_REGISTRY, COMPTROLLER, ACC1, BLOCK_NUMBER } = getContractAddresses(FORKED_NETWORK as string); |
There was a problem hiding this comment.
Guard unsupported FORKED_NETWORK values before destructuring.
Line 18 destructures getContractAddresses(...) at module load time, but that helper returns undefined for names missing from tests/hardhat/Fork/constants.ts. Since TIME_BASED_NETWORKS already includes several unsupported names, setting FORKED_NETWORK to one of them will crash before the if (FORK) guard runs.
Example guard
-const { POOL_REGISTRY, COMPTROLLER, ACC1, BLOCK_NUMBER } = getContractAddresses(FORKED_NETWORK as string);
+let POOL_REGISTRY!: string;
+let COMPTROLLER!: string;
+let ACC1: string | undefined;
+let BLOCK_NUMBER!: number;
+
+if (FORK) {
+ const addresses = getContractAddresses(FORKED_NETWORK);
+ if (!addresses) {
+ throw new Error(`Unsupported FORKED_NETWORK: ${FORKED_NETWORK}`);
+ }
+
+ ({ POOL_REGISTRY, COMPTROLLER, ACC1, BLOCK_NUMBER } = addresses);
+}Also applies to: 21-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hardhat/Fork/PoolLensForkTest.ts` around lines 15 - 18, The module
currently destructures POOL_REGISTRY, COMPTROLLER, ACC1, BLOCK_NUMBER from
getContractAddresses(FORKED_NETWORK) at load time which will throw if
getContractAddresses returns undefined for unsupported names; change to first
validate FORKED_NETWORK (e.g., check against the known network list or ensure
getContractAddresses(FORKED_NETWORK) !== undefined) or delay calling
getContractAddresses until inside the FORK guard, by assigning its return to a
temporary (e.g., addresses) and only destructuring POOL_REGISTRY, COMPTROLLER,
ACC1, BLOCK_NUMBER after confirming addresses is defined; ensure the same
pattern is applied to the other destructurings mentioned around lines 21–30.
Core pools should return all markets (including paused ones) since deprecated market filtering was only intended for isolated pools. Adds corePoolComptroller immutable to skip filtering for the core pool.
Add CORE_COMPTROLLER to fork test constants and verify that all markets are returned for core pools while deprecated filtering still applies to isolated pools.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/hardhat/Fork/PoolLensForkTest.ts (1)
18-20:⚠️ Potential issue | 🟠 MajorGuard
getContractAddresses(...)before destructuring.This still runs at module load time, so an unsupported
FORKED_NETWORKthrows before theif (FORK)guard can skip the suite. That is still easy to hit here becauseTIME_BASED_NETWORKSalready contains names thatgetContractAddresses(...)does not serve.Example fix
-const { POOL_REGISTRY, COMPTROLLER, CORE_COMPTROLLER, ACC1, BLOCK_NUMBER } = getContractAddresses( - FORKED_NETWORK as string, -); +let POOL_REGISTRY!: string; +let COMPTROLLER!: string; +let CORE_COMPTROLLER!: string; +let ACC1!: string; +let BLOCK_NUMBER!: number; + +if (FORK) { + const addresses = getContractAddresses(FORKED_NETWORK as string); + if (!addresses) { + throw new Error(`Unsupported FORKED_NETWORK: ${FORKED_NETWORK}`); + } + + ({ POOL_REGISTRY, COMPTROLLER, CORE_COMPTROLLER, ACC1, BLOCK_NUMBER } = addresses); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/PoolLensForkTest.ts` around lines 18 - 20, The destructuring call to getContractAddresses(...) is executed at module load and can throw for unsupported FORKED_NETWORK; instead, guard it before destructuring by first checking the FORK flag (or whether FORKED_NETWORK is supported) and only call getContractAddresses(...) inside that conditional, or call it into a temporary variable and validate it before extracting POOL_REGISTRY, COMPTROLLER, CORE_COMPTROLLER, ACC1, BLOCK_NUMBER; update references in PoolLensForkTest.ts so getContractAddresses is never invoked at module load time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/Lens/PoolLens.sol`:
- Around line 289-291: The code is filtering out unlisted/paused markets by
calling _getMarkets(...), which understates badDebt and totalBadDebtUsd; replace
the filtered call with the comptroller's full market list (call getAllMarkets()
on ComptrollerViewInterface(comptrollerAddress)) instead of
_getMarkets(ComptrollerInterface(comptrollerAddress)) so you iterate every
VToken returned (including unlisted/paused) and include each token's badDebt()
and its USD contribution when computing badDebts and totalBadDebtUsd
(referencing ComptrollerViewInterface, getAllMarkets, _getMarkets, VToken[],
comptrollerAddress, and badDebt()).
In `@deploy/007-deploy-pool-lens.ts`:
- Around line 16-21: The deployment currently calls deploy("PoolLens", ...) with
skipIfAlreadyDeployed: true which prevents updating existing PoolLens instances
after adding the new required constructor arg corePoolComptroller; update the
deploy logic so networks with an existing PoolLens get the new constructor
value: either remove skipIfAlreadyDeployed from the deploy(...) call to force
redeployment with the new constructorArgs (ensuring deployer/args are correct),
or implement an explicit upgrade path (use a proxy/upgrade mechanism to replace
PoolLens implementation while preserving storage) and wire corePoolComptroller
into the new implementation; locate references to PoolLens, constructorArgs,
corePoolComptroller, and skipIfAlreadyDeployed to make the change.
- Around line 13-14: Replace deployments.get("Comptroller_Core") with await
deployments.getOrNull("Comptroller_Core") and handle the null case: assign
corePoolComptrollerAddress = corePoolComptroller?.address ??
ethers.constants.AddressZero (or otherwise skip/queue this deploy) so
constructorArgs = [isTimeBased, blocksPerYear, corePoolComptrollerAddress];
ensure the code checks for a null corePoolComptroller returned by getOrNull
before accessing .address to avoid failures on first rollout.
---
Duplicate comments:
In `@tests/hardhat/Fork/PoolLensForkTest.ts`:
- Around line 18-20: The destructuring call to getContractAddresses(...) is
executed at module load and can throw for unsupported FORKED_NETWORK; instead,
guard it before destructuring by first checking the FORK flag (or whether
FORKED_NETWORK is supported) and only call getContractAddresses(...) inside that
conditional, or call it into a temporary variable and validate it before
extracting POOL_REGISTRY, COMPTROLLER, CORE_COMPTROLLER, ACC1, BLOCK_NUMBER;
update references in PoolLensForkTest.ts so getContractAddresses is never
invoked at module load time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c870f68-8512-44d5-9c0b-185450464f1b
📒 Files selected for processing (5)
contracts/Lens/PoolLens.soldeploy/007-deploy-pool-lens.tstests/hardhat/Fork/PoolLensForkTest.tstests/hardhat/Fork/constants.tstests/hardhat/Lens/PoolLens.ts
| // Get every listed market in the pool | ||
| ComptrollerViewInterface comptroller = ComptrollerViewInterface(comptrollerAddress); | ||
| VToken[] memory markets = comptroller.getAllMarkets(); | ||
| VToken[] memory markets = _getMarkets(ComptrollerInterface(comptrollerAddress)); |
There was a problem hiding this comment.
Keep bad-debt accounting on getAllMarkets().
A market can still carry badDebt() after it is unlisted or fully paused. Filtering with _getMarkets(...) here will understate both badDebts and totalBadDebtUsd for isolated pools—the exact markets most likely to have bad debt are the ones being dropped.
Suggested fix
- VToken[] memory markets = _getMarkets(ComptrollerInterface(comptrollerAddress));
+ VToken[] memory markets = comptroller.getAllMarkets();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get every listed market in the pool | |
| ComptrollerViewInterface comptroller = ComptrollerViewInterface(comptrollerAddress); | |
| VToken[] memory markets = comptroller.getAllMarkets(); | |
| VToken[] memory markets = _getMarkets(ComptrollerInterface(comptrollerAddress)); | |
| // Get every listed market in the pool | |
| ComptrollerViewInterface comptroller = ComptrollerViewInterface(comptrollerAddress); | |
| VToken[] memory markets = comptroller.getAllMarkets(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/Lens/PoolLens.sol` around lines 289 - 291, The code is filtering
out unlisted/paused markets by calling _getMarkets(...), which understates
badDebt and totalBadDebtUsd; replace the filtered call with the comptroller's
full market list (call getAllMarkets() on
ComptrollerViewInterface(comptrollerAddress)) instead of
_getMarkets(ComptrollerInterface(comptrollerAddress)) so you iterate every
VToken returned (including unlisted/paused) and include each token's badDebt()
and its USD contribution when computing badDebts and totalBadDebtUsd
(referencing ComptrollerViewInterface, getAllMarkets, _getMarkets, VToken[],
comptrollerAddress, and badDebt()).
| const corePoolComptroller = await deployments.get("Comptroller_Core"); | ||
| const constructorArgs = [isTimeBased, blocksPerYear, corePoolComptroller.address]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, explore the deploy directory structure and find related files
fd . deploy/ --type f --name "*.ts" | head -20Repository: VenusProtocol/isolated-pools
Length of output: 322
🏁 Script executed:
# Read the specific file mentioned in the review
wc -l deploy/007-deploy-pool-lens.tsRepository: VenusProtocol/isolated-pools
Length of output: 105
🏁 Script executed:
# Read the full context of deploy/007-deploy-pool-lens.ts
cat -n deploy/007-deploy-pool-lens.tsRepository: VenusProtocol/isolated-pools
Length of output: 1857
🏁 Script executed:
# Search for how Comptroller_Core is used in other deploy scripts
rg "Comptroller_Core" deploy/ -A 3 -B 3Repository: VenusProtocol/isolated-pools
Length of output: 598
🏁 Script executed:
# Check hardhat.config.ts to see network definitions and which are marked as live
cat -n hardhat.config.ts | head -100Repository: VenusProtocol/isolated-pools
Length of output: 5299
🏁 Script executed:
# Look for skipIfAlreadyDeployed pattern in other deploy scripts
rg "skipIfAlreadyDeployed" deploy/ -B 5 -A 2Repository: VenusProtocol/isolated-pools
Length of output: 13685
🏁 Script executed:
# Look for where Comptroller_Core is deployed
rg "Comptroller_Core" . --type ts -B 5 -A 5Repository: VenusProtocol/isolated-pools
Length of output: 9286
🏁 Script executed:
# Check deploy/008 which deploys comptrollers
cat -n deploy/008-deploy-comptrollers.tsRepository: VenusProtocol/isolated-pools
Length of output: 2554
🏁 Script executed:
# Search for getOrNull pattern in the codebase
rg "getOrNull|deployments\.get" deploy/ -A 2 -B 2Repository: VenusProtocol/isolated-pools
Length of output: 5685
🏁 Script executed:
# Check the deploymentConfig to see what pools are configured
rg "Core" helpers/deploymentConfig -A 3 -B 3Repository: VenusProtocol/isolated-pools
Length of output: 136
🏁 Script executed:
# Look for where pools are defined and if "Core" is a pool id
fd deploymentConfig.tsRepository: VenusProtocol/isolated-pools
Length of output: 99
🏁 Script executed:
# Check if there are references to which pools should be deployed
cat -n helpers/deploymentConfig.ts | head -150Repository: VenusProtocol/isolated-pools
Length of output: 7298
🏁 Script executed:
# Search for all pool definitions to understand pool naming
rg "pool.id|poolId" helpers/deploymentConfig.ts -B 2 -A 2Repository: VenusProtocol/isolated-pools
Length of output: 54
🏁 Script executed:
# Check what getUnregisteredPools does - it determines which comptrollers get deployed
rg "getUnregisteredPools" helpers/ -A 10Repository: VenusProtocol/isolated-pools
Length of output: 987
🏁 Script executed:
# Check deployment configurations for BSC networks to see if "Core" pool is defined
rg -A 500 "bscmainnet:" helpers/deploymentConfig.ts | head -150Repository: VenusProtocol/isolated-pools
Length of output: 4733
🏁 Script executed:
# Check bsctestnet config
rg -A 500 "bsctestnet:" helpers/deploymentConfig.ts | head -150Repository: VenusProtocol/isolated-pools
Length of output: 4680
🏁 Script executed:
# Look for where poolConfig gets the "Core" pool definition
rg "\"Core\"" helpers/deploymentConfig.tsRepository: VenusProtocol/isolated-pools
Length of output: 659
🏁 Script executed:
# Check all deployment configuration files
fd deploymentConfig -t fRepository: VenusProtocol/isolated-pools
Length of output: 99
🏁 Script executed:
# Look for where pool configs are actually defined with the Core pool
rg "id.*Core|Core.*poolConfig" . --type ts | head -20Repository: VenusProtocol/isolated-pools
Length of output: 934
🏁 Script executed:
# Check the full deploymentConfig to understand when Core pool is configured
wc -l helpers/deploymentConfig.tsRepository: VenusProtocol/isolated-pools
Length of output: 104
🏁 Script executed:
# Check the section with actual pool config definitions to see if Core is always configured
sed -n '150,500p' helpers/deploymentConfig.tsRepository: VenusProtocol/isolated-pools
Length of output: 13887
🏁 Script executed:
# Look at deploy scripts to understand execution order - hardhat-deploy executes in numeric order
ls -la deploy/ | grep "^-" | head -30Repository: VenusProtocol/isolated-pools
Length of output: 1866
🏁 Script executed:
# Check if deploy/007 depends on deploy/008's output
rg "dependsOn|tags.*il|func.tags" deploy/007-deploy-pool-lens.tsRepository: VenusProtocol/isolated-pools
Length of output: 103
🏁 Script executed:
# Check deploy/008 tags
rg "dependsOn|tags.*il|func.tags" deploy/008-deploy-comptrollers.tsRepository: VenusProtocol/isolated-pools
Length of output: 107
🏁 Script executed:
# Check if Comptroller_Core might be from external deployments in venus-protocol
rg "Comptroller_Core" node_modules/@venusprotocol/venus-protocol/deployments/ 2>/dev/null | head -20Repository: VenusProtocol/isolated-pools
Length of output: 54
🏁 Script executed:
# Verify hardhat-deploy execution order - does it execute by numeric filename?
# Check if there are any explicit dependencies between 007 and 008
rg "dependsOn" deploy/007-deploy-pool-lens.ts deploy/008-deploy-comptrollers.tsRepository: VenusProtocol/isolated-pools
Length of output: 54
🏁 Script executed:
# Look at how deploy/004-swap-router handles getting Comptroller_DeFi which is deployed in 008
cat -n deploy/004-swap-router.ts | head -50Repository: VenusProtocol/isolated-pools
Length of output: 2406
🏁 Script executed:
# Check if Comptroller_DeFi actually exists or if it's also from external deployments
rg "Comptroller_DeFi" helpers/deploymentConfig.ts -B 3 -A 3Repository: VenusProtocol/isolated-pools
Length of output: 54
🏁 Script executed:
# Search for how hardhat-deploy handles the execution when getting deployments that don't exist yet
# Check if there's any code that ensures deploy/008 runs before 007
rg "run\|tags\|il" deploy/ --type ts -B 2 -A 2 | grep -A 5 -B 5 "tags.*il"Repository: VenusProtocol/isolated-pools
Length of output: 54
🏁 Script executed:
# Look for any hardhat.config overrides for deployment order
rg "deploymentDependencies|namedAccounts|deploy.*order" hardhat.config.tsRepository: VenusProtocol/isolated-pools
Length of output: 90
🏁 Script executed:
# Check what happens when running deploy scripts - does deploy 008 need to run first?
# Look for any documentation or test that shows execution order
fd README -o DEPLOYMENT_ORDER -o deployment.mdRepository: VenusProtocol/isolated-pools
Length of output: 223
🏁 Script executed:
# Verify which pool names map to which comptroller deployments in deploy/008
grep -n "pool.id\|pool.name\|Comptroller_" deploy/008-deploy-comptrollers.tsRepository: VenusProtocol/isolated-pools
Length of output: 119
🏁 Script executed:
# Check if there's a specific way the scripts are meant to be run
cat -n package.json | grep -A 5 -B 5 "deploy"Repository: VenusProtocol/isolated-pools
Length of output: 2196
🏁 Script executed:
# Check if Comptroller_Core is provided by external venus-protocol deployments or only deployed locally
rg "Comptroller_Core" node_modules/@venusprotocol/ 2>/dev/null | head -5Repository: VenusProtocol/isolated-pools
Length of output: 54
🏁 Script executed:
# Look at the actual pool configurations to confirm Core pool is in every network's config
grep -A 50 "export const globalConfig" helpers/deploymentConfig.ts | grep -A 20 "bscmainnet:"Repository: VenusProtocol/isolated-pools
Length of output: 54
🏁 Script executed:
# Check if there's a deployment execution order documented or if tags affect order
cat -n deploy/006-deploy-pool-registry.ts | tail -10Repository: VenusProtocol/isolated-pools
Length of output: 284
🏁 Script executed:
# Key insight: Check if deploy scripts that have "il" tag all need to run in sequence
# See if any deployment script references both "il" tag and a previous numbered script's deployment
rg "func.tags.*il" deploy/ -B 2 -A 5Repository: VenusProtocol/isolated-pools
Length of output: 4164
🏁 Script executed:
# Check actual behavior - are there any snapshots or artifacts showing deployment order?
fd "deployments" --type d | head -5Repository: VenusProtocol/isolated-pools
Length of output: 84
🏁 Script executed:
# Final confirmation: Check if hardhat-deploy truly executes numerically
# Look at the actual behavior documentation in hardhat-deploy to confirm execution order
cat -n node_modules/hardhat-deploy/package.json | grep -A 2 -B 2 "version"Repository: VenusProtocol/isolated-pools
Length of output: 281
Use getOrNull() to handle missing Comptroller_Core on first deployment.
deployments.get("Comptroller_Core") will fail when deploying to live networks for the first time. Hardhat-deploy executes scripts numerically, so script 007 runs before script 008, which creates the Comptroller_Core deployment. On initial rollout, this artifact does not yet exist.
Possible fix
- const corePoolComptroller = await deployments.get("Comptroller_Core");
- const constructorArgs = [isTimeBased, blocksPerYear, corePoolComptroller.address];
+ const corePoolComptroller = await deployments.getOrNull("Comptroller_Core");
+ if (!corePoolComptroller) {
+ throw new Error("Comptroller_Core deployment not found. Run deploy/008-deploy-comptrollers.ts first.");
+ }
+ const constructorArgs = [isTimeBased, blocksPerYear, corePoolComptroller.address];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/007-deploy-pool-lens.ts` around lines 13 - 14, Replace
deployments.get("Comptroller_Core") with await
deployments.getOrNull("Comptroller_Core") and handle the null case: assign
corePoolComptrollerAddress = corePoolComptroller?.address ??
ethers.constants.AddressZero (or otherwise skip/queue this deploy) so
constructorArgs = [isTimeBased, blocksPerYear, corePoolComptrollerAddress];
ensure the code checks for a null corePoolComptroller returned by getOrNull
before accessing .address to avoid failures on first rollout.
| const poolLens = await deploy("PoolLens", { | ||
| from: deployer, | ||
| args: [isTimeBased, blocksPerYear], | ||
| args: constructorArgs, | ||
| log: true, | ||
| autoMine: true, | ||
| skipIfAlreadyDeployed: true, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the deploy script file
cat -n deploy/007-deploy-pool-lens.tsRepository: VenusProtocol/isolated-pools
Length of output: 1857
🏁 Script executed:
# Check if there are constructor args changes in this PR
git diff HEAD~1 deploy/007-deploy-pool-lens.ts 2>/dev/null || echo "Git diff not available, will check file directly"Repository: VenusProtocol/isolated-pools
Length of output: 120
🏁 Script executed:
# Search for any references to "core-pool exemption" in the codebase
rg "core-pool" --type ts --type js -iRepository: VenusProtocol/isolated-pools
Length of output: 54
🏁 Script executed:
# Check what PoolLens contract does and if constructor args relate to exemptions
rg "PoolLens" --type ts --type js -A 3 -B 3 | head -100Repository: VenusProtocol/isolated-pools
Length of output: 6575
🏁 Script executed:
# Search for the PoolLens contract definition to understand the constructor
fd "PoolLens.sol" -type fRepository: VenusProtocol/isolated-pools
Length of output: 242
🏁 Script executed:
# Search for "exemption" in the codebase to find what this refers to
rg "exemption" -i --type ts --type js --type sol -B 2 -A 2Repository: VenusProtocol/isolated-pools
Length of output: 99
🏁 Script executed:
# Check other deploy scripts for comparison with PoolLens deployment pattern
rg "skipIfAlreadyDeployed" deploy/ --type ts -B 5 -A 5 | head -80Repository: VenusProtocol/isolated-pools
Length of output: 5215
🏁 Script executed:
# Look for recent changes or PR context in version control
git log --oneline deploy/007-deploy-pool-lens.ts 2>/dev/null | head -5Repository: VenusProtocol/isolated-pools
Length of output: 139
🏁 Script executed:
# Find the PoolLens contract definition
find . -name "PoolLens.sol" -o -name "PoolLens.ts" | grep -v node_modules | grep -v testRepository: VenusProtocol/isolated-pools
Length of output: 101
🏁 Script executed:
# Search for corePoolComptroller to understand its usage
rg "corePoolComptroller" -B 3 -A 3Repository: VenusProtocol/isolated-pools
Length of output: 50387
🏁 Script executed:
# Check what the comment says in the git commit
git show 3ba76ad --stat 2>/dev/null | head -20Repository: VenusProtocol/isolated-pools
Length of output: 1065
🏁 Script executed:
# Look at the PoolLens contract to understand the constructor and what the core comptroller parameter does
rg "contract PoolLens" -A 30 --type-listRepository: VenusProtocol/isolated-pools
Length of output: 461
🏁 Script executed:
# Read the PoolLens.sol contract to understand the constructor and parameter
cat contracts/Lens/PoolLens.sol | head -150Repository: VenusProtocol/isolated-pools
Length of output: 5183
🏁 Script executed:
# Search specifically for corePoolComptroller or coreComptroller usage in PoolLens
rg "corePoolComptroller\|coreComptroller" contracts/Lens/PoolLens.sol -A 5 -B 2Repository: VenusProtocol/isolated-pools
Length of output: 54
🏁 Script executed:
# Look for the constructor in PoolLens
ast-grep --pattern 'contract PoolLens { $$$
constructor($_) { $$$
}
}'Repository: VenusProtocol/isolated-pools
Length of output: 54
🏁 Script executed:
# Search for any previous versions or references to PoolLens constructor without corePoolComptroller
rg "PoolLens\(" tests/ -B 2 -A 2 | head -40Repository: VenusProtocol/isolated-pools
Length of output: 54
🏁 Script executed:
# Check if there are any comments or git history about when corePoolComptroller was added
git log --all --oneline --grep="PoolLens" 2>/dev/null | head -10Repository: VenusProtocol/isolated-pools
Length of output: 659
🏁 Script executed:
# Search for the actual usage of corePoolComptroller in PoolLens to understand the behavior
rg "corePoolComptroller" contracts/Lens/PoolLens.sol -A 10 | head -60Repository: VenusProtocol/isolated-pools
Length of output: 1540
🏁 Script executed:
# Check if PoolLens was previously deployed and what the constructor looked like before this commit
git show 3ba76ad^:contracts/Lens/PoolLens.sol 2>/dev/null | grep -A 15 "constructor" | head -20Repository: VenusProtocol/isolated-pools
Length of output: 54
🏁 Script executed:
# Check deployments directory to see if PoolLens is already deployed on existing networks
ls -la deployments/ | grep -E "^d" | head -20Repository: VenusProtocol/isolated-pools
Length of output: 1227
🏁 Script executed:
# Check if PoolLens was deployed in prior deployment scripts
fd "deploy.*pool.*lens" deploy/ -iRepository: VenusProtocol/isolated-pools
Length of output: 102
🏁 Script executed:
# Verify if there's an existing deployment artifact for PoolLens
find deployments -name "PoolLens.json" 2>/dev/null | head -5Repository: VenusProtocol/isolated-pools
Length of output: 264
🏁 Script executed:
# Check what the previous version of PoolLens constructor looked like
git show 3ba76ad~1:contracts/Lens/PoolLens.sol 2>/dev/null | grep -A 20 "constructor" | head -25Repository: VenusProtocol/isolated-pools
Length of output: 54
🏁 Script executed:
# Check one of the existing deployment artifacts to see if it has the new constructor args
cat deployments/bscmainnet/PoolLens.json | jq '.args // .constructorArgs' 2>/dev/nullRepository: VenusProtocol/isolated-pools
Length of output: 95
🏁 Script executed:
# Get a clearer picture of what changed - check the diff for the PoolLens contract
git diff 3ba76ad~1 3ba76ad -- contracts/Lens/PoolLens.sol 2>/dev/null | head -80Repository: VenusProtocol/isolated-pools
Length of output: 54
🏁 Script executed:
# Let's check the full git commit to see what all changed
git show 3ba76ad --stat 2>/dev/null | head -30Repository: VenusProtocol/isolated-pools
Length of output: 1685
🏁 Script executed:
# Check if there's a deployment helper or if PoolLens gets deployed elsewhere or upgraded
rg "PoolLens" deploy/ --type ts -lRepository: VenusProtocol/isolated-pools
Length of output: 102
🏁 Script executed:
# Verify that skipIfAlreadyDeployed really does skip when contract exists
rg "skipIfAlreadyDeployed" -B 5 -A 5 --type ts | grep -A 10 "deploy\(" | head -40Repository: VenusProtocol/isolated-pools
Length of output: 95
🏁 Script executed:
# Check if there's an upgrade mechanism for PoolLens
rg "upgrade\|Upgrade" deploy/007-deploy-pool-lens.ts -iRepository: VenusProtocol/isolated-pools
Length of output: 54
Constructor signature change blocks deployment on networks with existing PoolLens.
The PR adds corePoolComptroller as a required constructor parameter (changing signature from 2 to 3 args), but skipIfAlreadyDeployed: true prevents redeployment on existing networks. PoolLens is already deployed on mainnet chains (BSC, Ethereum, Arbitrum, etc.) with the old 2-parameter constructor. When this deploy script runs on those networks, it will skip entirely, leaving the old deployment in place without the corePoolComptroller parameter. This means the core-pool market filtering logic won't activate on those networks.
Either remove skipIfAlreadyDeployed: true to allow re-deployment, or implement an upgrade path using a proxy if storage changes are a concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/007-deploy-pool-lens.ts` around lines 16 - 21, The deployment
currently calls deploy("PoolLens", ...) with skipIfAlreadyDeployed: true which
prevents updating existing PoolLens instances after adding the new required
constructor arg corePoolComptroller; update the deploy logic so networks with an
existing PoolLens get the new constructor value: either remove
skipIfAlreadyDeployed from the deploy(...) call to force redeployment with the
new constructorArgs (ensuring deployer/args are correct), or implement an
explicit upgrade path (use a proxy/upgrade mechanism to replace PoolLens
implementation while preserving storage) and wire corePoolComptroller into the
new implementation; locate references to PoolLens, constructorArgs,
corePoolComptroller, and skipIfAlreadyDeployed to make the change.
| VTOKEN1: ArbOneContracts.VToken_vWETH_Core.address, | ||
| VTOKEN2: ArbOneContracts.VToken_vARB_Core.address, | ||
| COMPTROLLER: ArbOneContracts.Comptroller_Core.address, | ||
| CORE_COMPTROLLER: ArbOneContracts.Comptroller_Core.address, |
| VTOKEN1: OpBnbMainnetContracts.VToken_vUSDT_Core.address, | ||
| VTOKEN2: OpBnbMainnetContracts.VToken_vFDUSD_Core.address, | ||
| COMPTROLLER: OpBnbMainnetContracts.Comptroller_Core.address, | ||
| CORE_COMPTROLLER: OpBnbMainnetContracts.Comptroller_Core.address, |
| VTOKEN1: EthereumContracts.VToken_vcrvUSD_Curve.address, | ||
| VTOKEN2: EthereumContracts.VToken_vCRV_Curve.address, | ||
| COMPTROLLER: EthereumContracts.Comptroller_Curve.address, | ||
| CORE_COMPTROLLER: EthereumContracts.Comptroller_Core.address, |
| if (address(comptroller) == corePoolComptroller) { | ||
| return comptroller.getAllMarkets(); | ||
| } | ||
| return _getActiveMarkets(comptroller); |
There was a problem hiding this comment.
i prefer return empty list instead of this as we already deprecated this part
…PoolLens All non-core pool markets are deprecated, so replace the active market filtering logic with an empty array return. Remove _getActiveMarkets and _isActiveMarket functions. Update fork tests accordingly and add base, optimism, and unichain network configs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/hardhat/Fork/constants.ts (1)
229-229: VerifyACC1placeholder address for Unichain.
ACC1is set toaddress(1)which is typically a precompile/system address. While the test has a fallback (ACC1 || AddressZero),getPendingRewardsmay return empty results if called with an account that has no positions. Consider using an actual account address with existing positions on Unichain mainnet for more meaningful test coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/constants.ts` at line 229, ACC1 is currently a placeholder (address(1)) which may produce empty results for getPendingRewards; update the ACC1 constant in constants.ts to a real Unichain mainnet address that holds positions/rewards (or a known funded test account on your fork) so getPendingRewards returns meaningful data, ensure the address is checksummed and keep the existing fallback (ACC1 || AddressZero) if needed for safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/hardhat/Fork/constants.ts`:
- Around line 214-231: The three new network objects (basemainnet, opmainnet,
unichainmainnet) are missing the COMPTROLLER key which PoolLensForkTest (it
destructures COMPTROLLER at multiple places) expects; add a COMPTROLLER property
to each network object in the constants (e.g., set COMPTROLLER to the same value
as CORE_COMPTROLLER if the network has a single core pool) so
PoolLensForkTest.ts can destructure and use COMPTROLLER without runtime errors.
---
Nitpick comments:
In `@tests/hardhat/Fork/constants.ts`:
- Line 229: ACC1 is currently a placeholder (address(1)) which may produce empty
results for getPendingRewards; update the ACC1 constant in constants.ts to a
real Unichain mainnet address that holds positions/rewards (or a known funded
test account on your fork) so getPendingRewards returns meaningful data, ensure
the address is checksummed and keep the existing fallback (ACC1 || AddressZero)
if needed for safety.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f723cc5-a0fc-4b3c-bb3e-c11635aa1b05
📒 Files selected for processing (3)
contracts/Lens/PoolLens.soltests/hardhat/Fork/PoolLensForkTest.tstests/hardhat/Fork/constants.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/Lens/PoolLens.sol
| basemainnet: { | ||
| CORE_COMPTROLLER: "0x0C7973F9598AA62f9e03B94E92C967fD5437426C", | ||
| POOL_REGISTRY: "0xeef902918DdeCD773D4B422aa1C6e1673EB9136F", | ||
| ACC1: "0xcdac0d6c6c59727a65f871236188350531885c43", | ||
| BLOCK_NUMBER: 44070000, | ||
| }, | ||
| opmainnet: { | ||
| CORE_COMPTROLLER: "0x5593FF68bE84C966821eEf5F0a988C285D5B7CeC", | ||
| POOL_REGISTRY: "0x147780799840d541C1d7c998F0cbA996d11D62bb", | ||
| ACC1: "0xacD03D601e5bB1B275Bb94076fF46ED9D753435A", | ||
| BLOCK_NUMBER: 149670000, | ||
| }, | ||
| unichainmainnet: { | ||
| CORE_COMPTROLLER: "0xe22af1e6b78318e1Fe1053Edbd7209b8Fc62c4Fe", | ||
| POOL_REGISTRY: "0x0C52403E16BcB8007C1e54887E1dFC1eC9765D7C", | ||
| ACC1: "0x0000000000000000000000000000000000000001", | ||
| BLOCK_NUMBER: 44190000, | ||
| }, |
There was a problem hiding this comment.
Missing COMPTROLLER field will cause test failures.
The new network entries (basemainnet, opmainnet, unichainmainnet) are missing the COMPTROLLER field, but PoolLensForkTest.ts destructures and uses it at multiple locations (lines 64, 80, 104, 127). Running tests with FORKED_NETWORK=basemainnet (or the other new networks) will cause runtime errors.
If these networks have a single core pool, COMPTROLLER should likely equal CORE_COMPTROLLER.
Proposed fix
basemainnet: {
CORE_COMPTROLLER: "0x0C7973F9598AA62f9e03B94E92C967fD5437426C",
+ COMPTROLLER: "0x0C7973F9598AA62f9e03B94E92C967fD5437426C",
POOL_REGISTRY: "0xeef902918DdeCD773D4B422aa1C6e1673EB9136F",
ACC1: "0xcdac0d6c6c59727a65f871236188350531885c43",
BLOCK_NUMBER: 44070000,
},
opmainnet: {
CORE_COMPTROLLER: "0x5593FF68bE84C966821eEf5F0a988C285D5B7CeC",
+ COMPTROLLER: "0x5593FF68bE84C966821eEf5F0a988C285D5B7CeC",
POOL_REGISTRY: "0x147780799840d541C1d7c998F0cbA996d11D62bb",
ACC1: "0xacD03D601e5bB1B275Bb94076fF46ED9D753435A",
BLOCK_NUMBER: 149670000,
},
unichainmainnet: {
CORE_COMPTROLLER: "0xe22af1e6b78318e1Fe1053Edbd7209b8Fc62c4Fe",
+ COMPTROLLER: "0xe22af1e6b78318e1Fe1053Edbd7209b8Fc62c4Fe",
POOL_REGISTRY: "0x0C52403E16BcB8007C1e54887E1dFC1eC9765D7C",
ACC1: "0x0000000000000000000000000000000000000001",
BLOCK_NUMBER: 44190000,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| basemainnet: { | |
| CORE_COMPTROLLER: "0x0C7973F9598AA62f9e03B94E92C967fD5437426C", | |
| POOL_REGISTRY: "0xeef902918DdeCD773D4B422aa1C6e1673EB9136F", | |
| ACC1: "0xcdac0d6c6c59727a65f871236188350531885c43", | |
| BLOCK_NUMBER: 44070000, | |
| }, | |
| opmainnet: { | |
| CORE_COMPTROLLER: "0x5593FF68bE84C966821eEf5F0a988C285D5B7CeC", | |
| POOL_REGISTRY: "0x147780799840d541C1d7c998F0cbA996d11D62bb", | |
| ACC1: "0xacD03D601e5bB1B275Bb94076fF46ED9D753435A", | |
| BLOCK_NUMBER: 149670000, | |
| }, | |
| unichainmainnet: { | |
| CORE_COMPTROLLER: "0xe22af1e6b78318e1Fe1053Edbd7209b8Fc62c4Fe", | |
| POOL_REGISTRY: "0x0C52403E16BcB8007C1e54887E1dFC1eC9765D7C", | |
| ACC1: "0x0000000000000000000000000000000000000001", | |
| BLOCK_NUMBER: 44190000, | |
| }, | |
| basemainnet: { | |
| CORE_COMPTROLLER: "0x0C7973F9598AA62f9e03B94E92C967fD5437426C", | |
| COMPTROLLER: "0x0C7973F9598AA62f9e03B94E92C967fD5437426C", | |
| POOL_REGISTRY: "0xeef902918DdeCD773D4B422aa1C6e1673EB9136F", | |
| ACC1: "0xcdac0d6c6c59727a65f871236188350531885c43", | |
| BLOCK_NUMBER: 44070000, | |
| }, | |
| opmainnet: { | |
| CORE_COMPTROLLER: "0x5593FF68bE84C966821eEf5F0a988C285D5B7CeC", | |
| COMPTROLLER: "0x5593FF68bE84C966821eEf5F0a988C285D5B7CeC", | |
| POOL_REGISTRY: "0x147780799840d541C1d7c998F0cbA996d11D62bb", | |
| ACC1: "0xacD03D601e5bB1B275Bb94076fF46ED9D753435A", | |
| BLOCK_NUMBER: 149670000, | |
| }, | |
| unichainmainnet: { | |
| CORE_COMPTROLLER: "0xe22af1e6b78318e1Fe1053Edbd7209b8Fc62c4Fe", | |
| COMPTROLLER: "0xe22af1e6b78318e1Fe1053Edbd7209b8Fc62c4Fe", | |
| POOL_REGISTRY: "0x0C52403E16BcB8007C1e54887E1dFC1eC9765D7C", | |
| ACC1: "0x0000000000000000000000000000000000000001", | |
| BLOCK_NUMBER: 44190000, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hardhat/Fork/constants.ts` around lines 214 - 231, The three new
network objects (basemainnet, opmainnet, unichainmainnet) are missing the
COMPTROLLER key which PoolLensForkTest (it destructures COMPTROLLER at multiple
places) expects; add a COMPTROLLER property to each network object in the
constants (e.g., set COMPTROLLER to the same value as CORE_COMPTROLLER if the
network has a single core pool) so PoolLensForkTest.ts can destructure and use
COMPTROLLER without runtime errors.
…urning empty list PoolLens._getMarkets now returns only markets with internalCash > 0 for non-core pools, avoiding reverts on deprecated markets while still exposing active ones. This removes the need to redeploy PoolLens after calling syncCash on remaining vTokens.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/hardhat/Fork/PoolLensForkTest.ts (1)
18-20:⚠️ Potential issue | 🟡 MinorGuard unsupported
FORKED_NETWORKvalues before destructuring.Line 18 destructures
getContractAddresses(...)at module load time, but that helper returnsundefinedfor network names missing fromconstants.ts. SinceTIME_BASED_NETWORKSincludes networks not incontractAddresses(e.g.,zksyncsepolia,opsepolia,basesepolia,unichainsepolia), settingFORKED_NETWORKto one of them will crash before theif (FORK)guard runs.Example guard
-const { POOL_REGISTRY, COMPTROLLER, CORE_COMPTROLLER, ACC1, BLOCK_NUMBER } = getContractAddresses( - FORKED_NETWORK as string, -); +let POOL_REGISTRY: string; +let COMPTROLLER: string | undefined; +let CORE_COMPTROLLER: string; +let ACC1: string | undefined; +let BLOCK_NUMBER: number; + +const addresses = getContractAddresses(FORKED_NETWORK as string); +if (addresses) { + ({ POOL_REGISTRY, COMPTROLLER, CORE_COMPTROLLER, ACC1, BLOCK_NUMBER } = addresses); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/PoolLensForkTest.ts` around lines 18 - 20, The test file destructures getContractAddresses(FORKED_NETWORK) at module load, which will crash when getContractAddresses returns undefined for unsupported networks; change the code to guard before destructuring by checking FORK and that FORKED_NETWORK is present in TIME_BASED_NETWORKS or that getContractAddresses(FORKED_NETWORK) returns a value, and only then call getContractAddresses and destructure POOL_REGISTRY, COMPTROLLER, CORE_COMPTROLLER, ACC1, BLOCK_NUMBER; alternatively, call getContractAddresses into a const (e.g., addresses) and validate addresses !== undefined and throw or skip tests with a clear message if missing to avoid the pre-guard crash.tests/hardhat/Fork/constants.ts (1)
214-231:⚠️ Potential issue | 🟡 MinorMissing
COMPTROLLERfield will cause test issues.The new network entries (
basemainnet,opmainnet,unichainmainnet) are missing theCOMPTROLLERfield, butPoolLensForkTest.tsdestructures and uses it at line 131. If these networks only have a single core pool,COMPTROLLERshould equalCORE_COMPTROLLER.Proposed fix
basemainnet: { CORE_COMPTROLLER: "0x0C7973F9598AA62f9e03B94E92C967fD5437426C", + COMPTROLLER: "0x0C7973F9598AA62f9e03B94E92C967fD5437426C", POOL_REGISTRY: "0xeef902918DdeCD773D4B422aa1C6e1673EB9136F", ACC1: "0xcdac0d6c6c59727a65f871236188350531885c43", BLOCK_NUMBER: 44070000, }, opmainnet: { CORE_COMPTROLLER: "0x5593FF68bE84C966821eEf5F0a988C285D5B7CeC", + COMPTROLLER: "0x5593FF68bE84C966821eEf5F0a988C285D5B7CeC", POOL_REGISTRY: "0x147780799840d541C1d7c998F0cbA996d11D62bb", ACC1: "0xacD03D601e5bB1B275Bb94076fF46ED9D753435A", BLOCK_NUMBER: 149670000, }, unichainmainnet: { CORE_COMPTROLLER: "0xe22af1e6b78318e1Fe1053Edbd7209b8Fc62c4Fe", + COMPTROLLER: "0xe22af1e6b78318e1Fe1053Edbd7209b8Fc62c4Fe", POOL_REGISTRY: "0x0C52403E16BcB8007C1e54887E1dFC1eC9765D7C", ACC1: "0x0000000000000000000000000000000000000001", BLOCK_NUMBER: 44190000, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/constants.ts` around lines 214 - 231, Add the missing COMPTROLLER properties for the new network entries (basemainnet, opmainnet, unichainmainnet) in the constants object so tests using COMPTROLLER (e.g., PoolLensForkTest.ts which destructures COMPTROLLER) don't break; set COMPTROLLER to the same value as CORE_COMPTROLLER for each entry when there is a single core pool (i.e., for basemainnet, opmainnet, and unichainmainnet assign COMPTROLLER = CORE_COMPTROLLER).contracts/Lens/PoolLens.sol (1)
289-291:⚠️ Potential issue | 🟠 MajorKeep bad-debt accounting on
getAllMarkets().Markets with
internalCash == 0can still carrybadDebt(). Using_getMarkets()here will understate bothbadDebtsandtotalBadDebtUsdfor non-core pools—the exact markets most likely to have bad debt are the ones being filtered out.Suggested fix
- VToken[] memory markets = _getMarkets(ComptrollerInterface(comptrollerAddress)); + VToken[] memory markets = ComptrollerViewInterface(comptrollerAddress).getAllMarkets();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/Lens/PoolLens.sol` around lines 289 - 291, The code uses _getMarkets(ComptrollerInterface(comptrollerAddress)) which filters out markets with internalCash == 0 and therefore omits markets that may have badDebt; replace that call with the full market list via getAllMarkets() on the comptroller (e.g., use ComptrollerViewInterface(comptrollerAddress).getAllMarkets() or otherwise call getAllMarkets() so the local variable markets includes every listed market), then continue computing badDebt() and totalBadDebtUsd over that complete markets array; update any references to _getMarkets in this context to avoid filtering out potentially bad-debt-carrying markets.
🧹 Nitpick comments (1)
tests/hardhat/Fork/PoolLensForkTest.ts (1)
68-127: Tests assume valid CORE_COMPTROLLER but some networks have zero address.Tests
getPoolByComptroller,getPendingRewards,getPoolBadDebt, andgetPoolDataFromVenusPoolall useCORE_COMPTROLLERdirectly. For networks whereCORE_COMPTROLLERis zero address (bsctestnet, bscmainnet), these tests will fail or produce meaningless results.Consider adding a guard at the suite level:
before(async () => { if (!CORE_COMPTROLLER || CORE_COMPTROLLER === ethers.constants.AddressZero) { console.log(`Skipping PoolLens fork tests: no CORE_COMPTROLLER for ${FORKED_NETWORK}`); return; } // ... rest of setup });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Fork/PoolLensForkTest.ts` around lines 68 - 127, The tests assume CORE_COMPTROLLER is a valid non-zero address but some forked networks set it to ethers.constants.AddressZero, causing meaningless failures in getPoolByComptroller, getPendingRewards, getPoolBadDebt and getPoolDataFromVenusPool; add a suite-level before(async () => { ... }) guard that checks if CORE_COMPTROLLER is falsy or equals ethers.constants.AddressZero, logs a message including FORKED_NETWORK, and returns early (skipping the rest of test setup) so those tests are not run on networks without a valid CORE_COMPTROLLER.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/hardhat/Fork/PoolLensForkTest.ts`:
- Line 65: The test unconditionally calls
Comptroller__factory.connect(CORE_COMPTROLLER, ethers.provider) which creates an
invalid instance when CORE_COMPTROLLER is the zero address; change the setup to
only call Comptroller__factory.connect when CORE_COMPTROLLER is not the zero
address (compare against ethers.constants.AddressZero) and set comptroller to
undefined/null otherwise, and then guard any tests that call
comptroller.getAllMarkets() (e.g. vTokenMetadataAll) by checking comptroller
exists and skipping/returning the test early when it does not.
---
Duplicate comments:
In `@contracts/Lens/PoolLens.sol`:
- Around line 289-291: The code uses
_getMarkets(ComptrollerInterface(comptrollerAddress)) which filters out markets
with internalCash == 0 and therefore omits markets that may have badDebt;
replace that call with the full market list via getAllMarkets() on the
comptroller (e.g., use
ComptrollerViewInterface(comptrollerAddress).getAllMarkets() or otherwise call
getAllMarkets() so the local variable markets includes every listed market),
then continue computing badDebt() and totalBadDebtUsd over that complete markets
array; update any references to _getMarkets in this context to avoid filtering
out potentially bad-debt-carrying markets.
In `@tests/hardhat/Fork/constants.ts`:
- Around line 214-231: Add the missing COMPTROLLER properties for the new
network entries (basemainnet, opmainnet, unichainmainnet) in the constants
object so tests using COMPTROLLER (e.g., PoolLensForkTest.ts which destructures
COMPTROLLER) don't break; set COMPTROLLER to the same value as CORE_COMPTROLLER
for each entry when there is a single core pool (i.e., for basemainnet,
opmainnet, and unichainmainnet assign COMPTROLLER = CORE_COMPTROLLER).
In `@tests/hardhat/Fork/PoolLensForkTest.ts`:
- Around line 18-20: The test file destructures
getContractAddresses(FORKED_NETWORK) at module load, which will crash when
getContractAddresses returns undefined for unsupported networks; change the code
to guard before destructuring by checking FORK and that FORKED_NETWORK is
present in TIME_BASED_NETWORKS or that getContractAddresses(FORKED_NETWORK)
returns a value, and only then call getContractAddresses and destructure
POOL_REGISTRY, COMPTROLLER, CORE_COMPTROLLER, ACC1, BLOCK_NUMBER; alternatively,
call getContractAddresses into a const (e.g., addresses) and validate addresses
!== undefined and throw or skip tests with a clear message if missing to avoid
the pre-guard crash.
---
Nitpick comments:
In `@tests/hardhat/Fork/PoolLensForkTest.ts`:
- Around line 68-127: The tests assume CORE_COMPTROLLER is a valid non-zero
address but some forked networks set it to ethers.constants.AddressZero, causing
meaningless failures in getPoolByComptroller, getPendingRewards, getPoolBadDebt
and getPoolDataFromVenusPool; add a suite-level before(async () => { ... })
guard that checks if CORE_COMPTROLLER is falsy or equals
ethers.constants.AddressZero, logs a message including FORKED_NETWORK, and
returns early (skipping the rest of test setup) so those tests are not run on
networks without a valid CORE_COMPTROLLER.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7803902a-a395-49e4-8ab8-7a659b11b791
📒 Files selected for processing (3)
contracts/Lens/PoolLens.soltests/hardhat/Fork/PoolLensForkTest.tstests/hardhat/Fork/constants.ts
| ); | ||
| await poolLens.deployed(); | ||
|
|
||
| comptroller = Comptroller__factory.connect(CORE_COMPTROLLER, ethers.provider); |
There was a problem hiding this comment.
Comptroller connection will fail for networks with zero-address CORE_COMPTROLLER.
For bsctestnet and bscmainnet, CORE_COMPTROLLER is 0x0000..., so connecting to it creates an invalid contract instance. Tests like vTokenMetadataAll (line 111) that call comptroller.getAllMarkets() will fail.
Proposed fix
- comptroller = Comptroller__factory.connect(CORE_COMPTROLLER, ethers.provider);
+ if (CORE_COMPTROLLER && CORE_COMPTROLLER !== ethers.constants.AddressZero) {
+ comptroller = Comptroller__factory.connect(CORE_COMPTROLLER, ethers.provider);
+ }Then guard tests that use comptroller with a check for its existence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hardhat/Fork/PoolLensForkTest.ts` at line 65, The test unconditionally
calls Comptroller__factory.connect(CORE_COMPTROLLER, ethers.provider) which
creates an invalid instance when CORE_COMPTROLLER is the zero address; change
the setup to only call Comptroller__factory.connect when CORE_COMPTROLLER is not
the zero address (compare against ethers.constants.AddressZero) and set
comptroller to undefined/null otherwise, and then guard any tests that call
comptroller.getAllMarkets() (e.g. vTokenMetadataAll) by checking comptroller
exists and skipping/returning the test early when it does not.
|
lgtm |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/hardhat/Lens/RewardsSummary.ts (1)
58-58: Consider adding a non-core fixture path in this suite.Both fixtures set
corePoolComptroller == comptroller, so this file only exercises the core branch. A small additional case with a differentcorePoolComptrollerwould improve unit coverage of the filtered-market branch.Also applies to: 171-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hardhat/Lens/RewardsSummary.ts` at line 58, The test currently only covers the core branch because both fixtures set corePoolComptroller == comptroller; add a second fixture in the RewardsSummary suite that deploys poolLens via poolLensFactory.deploy(isTimeBased, blocksPerYear, comptroller.address) but sets corePoolComptroller to a different address (e.g., deploy a mockComptroller or use an alternate signer) so the code path where corePoolComptroller != comptroller (the filtered-market branch) is exercised; create a small test that uses that non-core fixture and asserts behavior specific to the filtered-market branch to improve coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/hardhat/Lens/RewardsSummary.ts`:
- Line 58: The test currently only covers the core branch because both fixtures
set corePoolComptroller == comptroller; add a second fixture in the
RewardsSummary suite that deploys poolLens via
poolLensFactory.deploy(isTimeBased, blocksPerYear, comptroller.address) but sets
corePoolComptroller to a different address (e.g., deploy a mockComptroller or
use an alternate signer) so the code path where corePoolComptroller !=
comptroller (the filtered-market branch) is exercised; create a small test that
uses that non-core fixture and asserts behavior specific to the filtered-market
branch to improve coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 189a741f-be48-4fe3-ae19-cd109535f0f0
📒 Files selected for processing (15)
deployments/arbitrumone/PoolLens.jsondeployments/arbitrumone/solcInputs/09f8b2889a382752688c03578bc27f8d.jsondeployments/basemainnet/PoolLens.jsondeployments/basemainnet/solcInputs/09f8b2889a382752688c03578bc27f8d.jsondeployments/ethereum/PoolLens.jsondeployments/ethereum/solcInputs/09f8b2889a382752688c03578bc27f8d.jsondeployments/opbnbmainnet/PoolLens.jsondeployments/opbnbmainnet/solcInputs/09f8b2889a382752688c03578bc27f8d.jsondeployments/opmainnet/PoolLens.jsondeployments/opmainnet/solcInputs/09f8b2889a382752688c03578bc27f8d.jsondeployments/unichainmainnet/PoolLens.jsondeployments/unichainmainnet/solcInputs/09f8b2889a382752688c03578bc27f8d.jsondeployments/zksyncmainnet/PoolLens.jsondeployments/zksyncmainnet/solcInputs/10818118264cbc82e2eea514eb3f075b.jsontests/hardhat/Lens/RewardsSummary.ts
|
|
We won't merge this, as its for a temporary usage |
fred-venus
changed the title
2 participants
Summary
_getActiveMarketsfilter that excludes unlisted markets and markets with both MINT and BORROW actions pausedChanges
PoolLens (
contracts/Lens/PoolLens.sol)_getActiveMarkets()helper that filtersgetAllMarkets()in a single loop, excluding markets whereisListed == falseor both MINT and BORROW are paused_isActiveMarket()helper encapsulating the active market checkgetPoolDataFromVenusPool(),getPendingRewards(), andgetPoolBadDebt()to use_getActiveMarkets()instead ofgetAllMarkets()Fork test (
tests/hardhat/Fork/PoolLensForkTest.ts)getAllPools,getPoolByComptroller,getPendingRewards,getPoolBadDebt,vTokenMetadataAll,getPoolDataFromVenusPoolSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores