[VPD-485]: Add VIP to Configure RelativePositionManager#671
Open
[VPD-485]: Add VIP to Configure RelativePositionManager#671
Conversation
|
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:
📝 WalkthroughWalkthroughAdds VIP-610: many new contract ABIs, VIP proposal modules for BSC mainnet/testnet, and extensive Hardhat forked simulation tests that execute the proposal and exercise governance, ACM permission changes, PositionAccount registration, and end-to-end RelativePositionManager/leverage/flash flows. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(10,160,220,0.5)
actor Proposer
end
rect rgba(60,180,75,0.5)
participant Timelock as Timelock / NORMAL_TIMELOCK
end
rect rgba(200,100,40,0.5)
participant ACM as AccessControlManager
participant RPM as RelativePositionManager
participant LSM as LeverageStrategiesManager
participant SH as SwapHelper
participant Oracle as Oracle
participant VToken as VToken
participant Flash as FlashLoanFacet
participant Position as PositionAccount
end
Proposer->>Timelock: submit & execute VIP-610 proposal
Timelock->>ACM: giveCallPermission(fnSigs...) to timelocks/guardians
Timelock->>RPM: acceptOwnership / setPositionAccountImplementation / addDSAVToken
Timelock->>LSM: acceptOwnership / whitelist flash-loan (testnet)
Timelock->>SH: acceptOwnership
ACM->>RPM: permit governed calls (pause/unpause/setters/exec)
User->>RPM: activateAndOpenPosition
RPM->>SH: perform swap via genericCall/multicall
RPM->>Oracle: getPrice / (tests) setPrice
RPM->>Flash: request flash loan
Flash->>VToken: transfer/repay during loan
RPM->>Position: executePositionAccountCall / position lifecycle
RPM->>Proposer: emits events (RoleGranted, OwnershipTransferred, PositionOpened/Closed)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
RPM_VIP_SIMULATION_GUIDE.md (1)
67-81: Add language specifier to fenced code block.The file structure diagram should include a language identifier to satisfy markdown lint rules and improve rendering consistency.
📝 Proposed fix
-``` +```text vips/ ├── vips/vip-XXX/ │ └── bscmainnet.ts # VIP definition (if RPM config changes needed)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RPM_VIP_SIMULATION_GUIDE.md` around lines 67 - 81, Update the fenced code block that contains the directory tree beginning with "vips/" to include a language specifier (e.g., replace the opening ``` with ```text) so the file structure block is lint-compliant and renders consistently; modify the fenced block around the vips/ tree in RPM_VIP_SIMULATION_GUIDE.md to add the specifier.
🤖 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-610/bscmainnet.ts`:
- Line 199: Replace the CommonJS require for VTOKEN_ABI with an ES module import
to satisfy `@typescript-eslint/no-var-requires`: move the import of
"./abi/VToken.json" to the top of the file alongside the other ABI imports and
add a statement like `import VTOKEN_ABI from "./abi/VToken.json";` (use the same
import pattern used for the other ABIs in this file), then remove the inline
`const VTOKEN_ABI = require("./abi/VToken.json");` line; ensure the symbol
VTOKEN_ABI and the referenced file VToken.json are used unchanged elsewhere.
---
Nitpick comments:
In `@RPM_VIP_SIMULATION_GUIDE.md`:
- Around line 67-81: Update the fenced code block that contains the directory
tree beginning with "vips/" to include a language specifier (e.g., replace the
opening ``` with ```text) so the file structure block is lint-compliant and
renders consistently; modify the fenced block around the vips/ tree in
RPM_VIP_SIMULATION_GUIDE.md to add the specifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b34d34dd-140a-4b5b-8b0c-eb4ad7d8396e
📒 Files selected for processing (14)
RPM_VIP_SIMULATION_GUIDE.mdsimulations/vip-610/abi/AccessControlManager.jsonsimulations/vip-610/abi/BinanceOracle.jsonsimulations/vip-610/abi/ChainlinkOracle.jsonsimulations/vip-610/abi/FlashLoanFacet.jsonsimulations/vip-610/abi/LeverageStrategiesManager.jsonsimulations/vip-610/abi/RelativePositionManager.jsonsimulations/vip-610/abi/ResilientOracle.jsonsimulations/vip-610/abi/SwapHelperAbi.jsonsimulations/vip-610/abi/VToken.jsonsimulations/vip-610/bscmainnet.tssimulations/vip-610/bsctestnet.tsvips/vip-610/bscmainnet.tsvips/vip-610/bsctestnet.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
simulations/vip-610/bscmainnet.ts (2)
102-104: Minor inconsistency: Use.to.equal()for consistency.Line 103 uses
.equals()while the rest of the file uses.to.equal(). Both work, but consistency improves readability.♻️ Suggested fix
it("RPM should have Position Account implementation stored in the state", async () => { - expect(await relativePositionManager.POSITION_ACCOUNT_IMPLEMENTATION()).to.equals(POSITION_ACCOUNT); + expect(await relativePositionManager.POSITION_ACCOUNT_IMPLEMENTATION()).to.equal(POSITION_ACCOUNT); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-610/bscmainnet.ts` around lines 102 - 104, The test uses Chai's .equals() on the assertion for relativePositionManager.POSITION_ACCOUNT_IMPLEMENTATION() but the rest of the file uses .to.equal() for consistency; update the assertion in the test case ("RPM should have Position Account implementation stored in the state") to use .to.equal(POSITION_ACCOUNT) instead of .equals(), keeping the same expectation call to relativePositionManager.POSITION_ACCOUNT_IMPLEMENTATION().
1063-1069: Confusing variable naming:liquidatoris actually the position owner.The variable named
liquidator(line 1064) is used as the position owner who gets liquidated, whilealiceperforms the actual liquidation. Consider renaming for clarity.♻️ Suggested fix
it("liquidate position after price crash", async function () { - const [, , , liquidator] = await ethers.getSigners(); + const [, , , positionOwner] = await ethers.getSigners(); const INITIAL_PRINCIPAL = ethers.utils.parseEther("1500");Then update all references to
liquidatorwithin this test topositionOwner(e.g., lines 1072, 1074, 1086, 1098, 1099).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-610/bscmainnet.ts` around lines 1063 - 1069, In the "liquidate position after price crash" test, the signer variable named `liquidator` is actually the position owner who gets liquidated; rename `liquidator` to `positionOwner` and update every reference to it within that test (e.g., where it's used after ethers.getSigners() and at usages around INITIAL_PRINCIPAL, SHORT_AMOUNT, LONG_AMOUNT, leverage and subsequent calls) so that `alice` remains the actor performing the liquidation and the semantics are clear; adjust all occurrences of `liquidator` in this test function to `positionOwner`.
🤖 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-610/bscmainnet.ts`:
- Around line 102-104: The test uses Chai's .equals() on the assertion for
relativePositionManager.POSITION_ACCOUNT_IMPLEMENTATION() but the rest of the
file uses .to.equal() for consistency; update the assertion in the test case
("RPM should have Position Account implementation stored in the state") to use
.to.equal(POSITION_ACCOUNT) instead of .equals(), keeping the same expectation
call to relativePositionManager.POSITION_ACCOUNT_IMPLEMENTATION().
- Around line 1063-1069: In the "liquidate position after price crash" test, the
signer variable named `liquidator` is actually the position owner who gets
liquidated; rename `liquidator` to `positionOwner` and update every reference to
it within that test (e.g., where it's used after ethers.getSigners() and at
usages around INITIAL_PRINCIPAL, SHORT_AMOUNT, LONG_AMOUNT, leverage and
subsequent calls) so that `alice` remains the actor performing the liquidation
and the semantics are clear; adjust all occurrences of `liquidator` in this test
function to `positionOwner`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf178de5-cf45-41ef-a393-9295fc0f88e8
📒 Files selected for processing (1)
simulations/vip-610/bscmainnet.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
simulations/vip-610/bscmainnet.ts (2)
218-219: Consider adding explicit types for test signers.Using
anytype foraliceandbobloses type safety. Consider usingSignerWithAddressfrom@nomiclabs/hardhat-ethers/signers.♻️ Proposed fix
+import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; - let alice: any; - let bob: any; + let alice: SignerWithAddress; + let bob: SignerWithAddress;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-610/bscmainnet.ts` around lines 218 - 219, The variables `alice` and `bob` are declared as `any`, losing type safety; change their declarations to use the proper signer type (e.g., import and use `SignerWithAddress` from `@nomiclabs/hardhat-ethers/signers`) and type them as `let alice: SignerWithAddress; let bob: SignerWithAddress;`, updating any usages in the file (e.g., where they are assigned in test setup) to match the typed signatures.
65-80: ACM permission check useshasRolewith computed roleHash.This approach computes the role hash as
keccak256(solidityPack([address, string], [contract, fnSignature]))while the testnet simulation useshasPermission(account, contract, fnSignature)directly. Both approaches are valid but the inconsistency may cause confusion when comparing test files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-610/bscmainnet.ts` around lines 65 - 80, The test inconsistently checks ACM permissions by computing a roleHash and calling accessControlManager.hasRole(...) while other sims use accessControlManager.hasPermission(account, contract, fnSignature); update this test to use hasPermission for clarity and consistency: inside the loops call await accessControlManager.hasPermission(timelockOrGuardian, RELATIVE_POSITION_MANAGER, fnSignature) and assert false, and replace the setPositionAccountImplementation keccak logic with await accessControlManager.hasPermission(bscmainnet.NORMAL_TIMELOCK, RELATIVE_POSITION_MANAGER, "setPositionAccountImplementation(address)") asserted false; keep TIMELOCKS_AND_GUARDIAN, ACM_FUNCTION_SIGNATURES and RELATIVE_POSITION_MANAGER identifiers unchanged.
🤖 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-610/bscmainnet.ts`:
- Around line 191-199: DSA_WHALE is incorrectly set to the USDC token contract
(DSA_ADDRESS) which holds zero token balance; change DSA_WHALE to a real USDC
whale address (similar to SHORT_WHALE) and stop hardcoding addresses here—import
the canonical addresses from src/networkAddresses.ts (use the exported
USDC/whale identifiers) and replace the local constants (DSA_WHALE, DSA_ADDRESS,
vDSA_ADDRESS, etc.) with those imports so tests use an account with an actual
USDC balance rather than the token contract.
---
Nitpick comments:
In `@simulations/vip-610/bscmainnet.ts`:
- Around line 218-219: The variables `alice` and `bob` are declared as `any`,
losing type safety; change their declarations to use the proper signer type
(e.g., import and use `SignerWithAddress` from
`@nomiclabs/hardhat-ethers/signers`) and type them as `let alice:
SignerWithAddress; let bob: SignerWithAddress;`, updating any usages in the file
(e.g., where they are assigned in test setup) to match the typed signatures.
- Around line 65-80: The test inconsistently checks ACM permissions by computing
a roleHash and calling accessControlManager.hasRole(...) while other sims use
accessControlManager.hasPermission(account, contract, fnSignature); update this
test to use hasPermission for clarity and consistency: inside the loops call
await accessControlManager.hasPermission(timelockOrGuardian,
RELATIVE_POSITION_MANAGER, fnSignature) and assert false, and replace the
setPositionAccountImplementation keccak logic with await
accessControlManager.hasPermission(bscmainnet.NORMAL_TIMELOCK,
RELATIVE_POSITION_MANAGER, "setPositionAccountImplementation(address)") asserted
false; keep TIMELOCKS_AND_GUARDIAN, ACM_FUNCTION_SIGNATURES and
RELATIVE_POSITION_MANAGER identifiers unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5501ec46-1e02-4b76-8964-41210d158689
📒 Files selected for processing (4)
simulations/vip-610/bscmainnet.tssimulations/vip-610/bsctestnet.tsvips/vip-610/bscmainnet.tsvips/vip-610/bsctestnet.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- vips/vip-610/bscmainnet.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
simulations/vip-610/bscmainnet.ts (1)
198-199:⚠️ Potential issue | 🔴 CriticalDSA_WHALE incorrectly set to token contract address—tests will fail.
DSA_WHALEis set toDSA_ADDRESS(the USDC token contract at0x8AC76a51cc950d9822D68b83fE1Ad97B32Cd580d). Token contract addresses do not hold their own tokens; the contract maintains balances via internal mappings but the address itself has zero balance. All E2E tests usingDSA_WHALEto fund users (lines 378, 495, 608, 731, 851, 957, 1090, 1198, 1327) will fail.Use a known USDC whale address similar to
SHORT_WHALE(Binance hot wallet).🐛 Proposed fix
- const DSA_WHALE = DSA_ADDRESS; + const DSA_WHALE = "0x8894E0a0c962CB723c1976a4421c95949bE2D4E3"; // Binance hot wallet (holds USDC)Or use another verified USDC whale address on BSC mainnet.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-610/bscmainnet.ts` around lines 198 - 199, DSA_WHALE is incorrectly set to the token contract constant DSA_ADDRESS; replace its value with a real externally owned USDC whale address (for example reuse SHORT_WHALE or another verified BSC USDC whale) so calls that use DSA_WHALE to fund users (references in this file where DSA_WHALE is used to seed balances) will succeed; update the DSA_WHALE constant assignment (and remove any assignment to DSA_ADDRESS) to the chosen whale address and run the E2E funding flows to verify successful transfers.
🧹 Nitpick comments (1)
simulations/vip-610/bscmainnet.ts (1)
256-260: Default whale fallback may fail for token contracts.When
tokenOutWhaleOverrideis not provided,tokenOutWhaledefaults totokenOut. IftokenOutis a token contract address (which has zero balance of its own tokens), the subsequent transfer at line 260 will fail.Consider adding a validation or clearer documentation. The test at line 1096-1102 (
LONG = DSAcase) calls this function withDSA_ADDRESSastokenOutwithout an override—verify it works or provideDSA_WHALEas the override (onceDSA_WHALEis fixed).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-610/bscmainnet.ts` around lines 256 - 260, The fallback assigning tokenOutWhale = tokenOut when tokenOutWhaleOverride is empty can point to the token contract itself (which has no token balance) and cause the transfer in tokenOutContract.connect(...).transfer(...) to fail; update the logic in the block that builds tokenOutWhale (using symbols tokenOutWhaleOverride, tokenOut, initMainnetUser, tokenOutContract, transfer, amountOut) to first resolve a real holder: either require/throw if no explicit whale override is provided, or look up a known whale mapping (e.g., DSA_WHALE for DSA_ADDRESS) and use that, and additionally validate the resolved whale has a sufficient tokenOut balance before calling initMainnetUser and transfer; if validation fails, surface a clear error asking the caller to pass tokenOutWhaleOverride.
🤖 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-610/bscmainnet.ts`:
- Line 1: The import line 'import { expect } from "chai";' (and the rest of this
file) is failing Prettier; run the project's formatter (yarn prettier --write)
on this file or apply the project's Prettier rules so the file matches CI
formatting (fix whitespace, trailing newline, semicolons/quotes per config) and
commit the formatted result.
---
Duplicate comments:
In `@simulations/vip-610/bscmainnet.ts`:
- Around line 198-199: DSA_WHALE is incorrectly set to the token contract
constant DSA_ADDRESS; replace its value with a real externally owned USDC whale
address (for example reuse SHORT_WHALE or another verified BSC USDC whale) so
calls that use DSA_WHALE to fund users (references in this file where DSA_WHALE
is used to seed balances) will succeed; update the DSA_WHALE constant assignment
(and remove any assignment to DSA_ADDRESS) to the chosen whale address and run
the E2E funding flows to verify successful transfers.
---
Nitpick comments:
In `@simulations/vip-610/bscmainnet.ts`:
- Around line 256-260: The fallback assigning tokenOutWhale = tokenOut when
tokenOutWhaleOverride is empty can point to the token contract itself (which has
no token balance) and cause the transfer in
tokenOutContract.connect(...).transfer(...) to fail; update the logic in the
block that builds tokenOutWhale (using symbols tokenOutWhaleOverride, tokenOut,
initMainnetUser, tokenOutContract, transfer, amountOut) to first resolve a real
holder: either require/throw if no explicit whale override is provided, or look
up a known whale mapping (e.g., DSA_WHALE for DSA_ADDRESS) and use that, and
additionally validate the resolved whale has a sufficient tokenOut balance
before calling initMainnetUser and transfer; if validation fails, surface a
clear error asking the caller to pass tokenOutWhaleOverride.
🪄 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: 92b6dc1c-2951-43b0-8573-9e3c3f7bb817
📒 Files selected for processing (3)
simulations/vip-610/abi/RelativePositionManager.jsonsimulations/vip-610/bscmainnet.tsvips/vip-610/bscmainnet.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- simulations/vip-610/abi/RelativePositionManager.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
simulations/vip-610/bscmainnet.ts (1)
1311-1322: Consider renamingliquidatorvariable topositionOwnerfor clarity.The variable
liquidator(line 1312) is used as the position owner, whilealiceperforms the actual liquidation. The comment at line 1321 clarifies this, but renaming would improve readability:- const [, , , liquidator] = await ethers.getSigners(); + const [, , , positionOwner] = await ethers.getSigners();Then update all references (
liquidator.address→positionOwner.address,liquidator)→positionOwner)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulations/vip-610/bscmainnet.ts` around lines 1311 - 1322, The variable name liquidator is misleading because it is actually used as the position owner; rename the identifier liquidator to positionOwner across this test to improve clarity: update the variable declaration (const [, , , liquidator] → const [, , , positionOwner]) and all subsequent uses (positionOwner.address, dsa.connect(positionOwner), and any other references) including the comment that currently explains the role; ensure tests still pass by preserving the same signer object and only changing the identifier.
🤖 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-610/bscmainnet.ts`:
- Around line 1311-1322: The variable name liquidator is misleading because it
is actually used as the position owner; rename the identifier liquidator to
positionOwner across this test to improve clarity: update the variable
declaration (const [, , , liquidator] → const [, , , positionOwner]) and all
subsequent uses (positionOwner.address, dsa.connect(positionOwner), and any
other references) including the comment that currently explains the role; ensure
tests still pass by preserving the same signer object and only changing the
identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef51480b-e954-41c5-a427-63829a90630c
📒 Files selected for processing (3)
simulations/vip-610/abi/PositionAccount.jsonsimulations/vip-610/abi/RelativePositionManager.jsonsimulations/vip-610/bscmainnet.ts
✅ Files skipped from review due to trivial changes (1)
- simulations/vip-610/abi/PositionAccount.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Resolves VEN
Summary by CodeRabbit
New Features
Tests