feat: gateway v4 strategies #741
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces two new strategy contracts (SolvBTCJUPStrategy, SolvBTCPlusStrategy) and a v1 router interface, adds a Forge deployment script, updates constants and forked testing templates, switches certain tests to use WBTC_OFT, and adds new end-to-end tests for the new strategies with updated fork parameters. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Gateway/Caller
participant Strat as SolvBTCJUPStrategy
participant V2 as SolvBTCRouterV2
participant V1 as SolvBTCRouterV1
participant solvBTC as ERC20(solvBTC)
participant jup as ERC20(solvBTC.JUP)
Caller->>Strat: handleGatewayMessageWithSlippageArgs(args)
Strat->>solvBTC: transferFrom(Caller, Strat, amountIn)
Strat->>solvBTC: approve(V2, amountIn)
Strat->>V2: deposit(solvBTC, amountIn, expiry=now+1s)
V2-->>Strat: minted solvBTC (amountOut)
Strat->>solvBTC: approve(V1, amountOut)
Strat->>V1: createSubscription(poolId, amountOut)
V1-->>Strat: shareValue (implicit)
Strat->>jup: transfer(Recipient, jupAmount)
Strat-->>Caller: emit Output(jupAmount)
sequenceDiagram
autonumber
actor Caller as Gateway/Caller
participant Strat as SolvBTCPlusStrategy
participant V2 as SolvBTCRouterV2
participant plus as ERC20(solvBTCPlus)
participant wbtc as ERC20(WBTC/OFT)
Caller->>Strat: handleGatewayMessageWithSlippageArgs(args)
Strat->>wbtc: transferFrom(Caller, Strat, amountIn)
Strat->>wbtc: approve(V2, amountIn)
Strat->>V2: depositToPlus(amountIn, expiry=now+1s)
V2-->>Strat: minted solvBTCPlus (amountOut)
Strat->>plus: transfer(Recipient, amountOut)
Strat-->>Caller: emit Output(amountOut)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Orland0x, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request expands the gateway's capabilities by integrating new strategies for interacting with SolvBTC protocols. It introduces specific contracts to handle deposits and subscriptions for SolvBTC.JUP and SolvBTCPlus, enhancing the range of supported assets and investment pathways within the system. The changes also include necessary updates to the testing framework to ensure the robustness and correctness of these new integrations.
Highlights
- New SolvBTC Strategies: Introduced two new strategies:
SolvBTCJUPStrategyfor depositing into SolvBTC via RouterV2 and then subscribing to SolvBTC.JUP via RouterV1, andSolvBTCPlusStrategyfor depositing directly into SolvBTCPlus via RouterV2. - SolvBTCRouterV1 Interface: Added the
ISolvBTCRouterV1interface to support interactions with SolvBTC's older router for specific subscription functionalities. - Deployment Script for New Strategies: A new deployment script,
DeployStrategy.s.sol, was added to facilitate the deployment of theSolvBTCJUPStrategycontract, with corresponding broadcast logs. - Updated Test Infrastructure: Modified existing test files (
BedrockStrategyForked.sol,SolvStrategyForked.sol,ForkedTemplate.sol,Constants.sol) to incorporate a newForkedStrategyTemplateWbtcOftfor testing with WBTC OFT and to include dedicated tests for the newly added SolvBTC strategies.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces two new gateway strategies, SolvBTCJUPStrategy and SolvBTCPlusStrategy, along with corresponding tests and deployment information. The changes are well-structured. My review focuses on improving contract robustness, completeness of deployment scripts, and test code quality. I've suggested adding sweep functions to prevent stuck funds, making deadlines more flexible, completing the deployment script, and refactoring test helpers to reduce duplication. There's also a point about a high slippage tolerance in one of the tests that might need clarification.
| import {IERC20} from "@openzeppelin/contracts/interfaces/IERC20.sol"; | ||
|
|
||
| contract DeployStrategyScript is Script { | ||
| address public deployer; |
There was a problem hiding this comment.
The state variable deployer is declared but never used. It should be removed to improve code clarity and reduce contract size.
| function run() public { | ||
| uint256 deployerPrivateKey = vm.envUint("PRIVATE_KEY"); | ||
|
|
||
| vm.startBroadcast(deployerPrivateKey); | ||
|
|
||
| ISolvBTCRouterV2 solvBTCRouterV2 = ISolvBTCRouterV2(0x56a4d805d7A292f03Ead5Be31E0fFB8f7d0E3B48); | ||
| ISolvBTCRouterV1 solvBTCRouterV1 = ISolvBTCRouterV1(0xbA46FcC16B464D9787314167bDD9f1Ce28405bA1); | ||
| IERC20 solvBTC = IERC20(0x541FD749419CA806a8bc7da8ac23D346f2dF8B77); | ||
| IERC20 solvBTCJUP = IERC20(0x6b062AA7F5FC52b530Cb13967aE2E6bc0D8Dd3E4); | ||
| bytes32 poolId = 0x6f113a39a50769de40d4f2e7e46b6a4c6d7774e2c3943ced2dbcb25e626d1d04; | ||
|
|
||
| new SolvBTCJUPStrategy(solvBTCRouterV1, solvBTCRouterV2, poolId, solvBTC, solvBTCJUP); | ||
|
|
||
| vm.stopBroadcast(); | ||
| } |
There was a problem hiding this comment.
This deployment script only deploys the SolvBTCJUPStrategy. However, the PR introduces two new strategies (SolvBTCJUPStrategy and SolvBTCPlusStrategy). For completeness and to match what seems to be intended (based on broadcast JSON files), this script should also deploy SolvBTCPlusStrategy.
| contract SolvBTCJUPStrategy is IStrategyWithSlippageArgs, Context { | ||
| using SafeERC20 for IERC20; | ||
|
|
||
| ISolvBTCRouterV1 public immutable solvBTCRouterV1; | ||
| ISolvBTCRouterV2 public immutable solvBTCRouterV2; | ||
| bytes32 public immutable poolId; | ||
| IERC20 public immutable solvBTC; | ||
| IERC20 public immutable solvBTCJUP; | ||
|
|
||
| constructor( | ||
| ISolvBTCRouterV1 _solvBTCRouterV1, | ||
| ISolvBTCRouterV2 _solvBTCRouterV2, | ||
| bytes32 _poolId, | ||
| IERC20 _solvBTC, | ||
| IERC20 _solvBTCJUP | ||
| ) { | ||
| solvBTCRouterV1 = _solvBTCRouterV1; | ||
| solvBTCRouterV2 = _solvBTCRouterV2; | ||
| poolId = _poolId; | ||
| solvBTC = _solvBTC; | ||
| solvBTCJUP = _solvBTCJUP; | ||
| } | ||
|
|
||
| function handleGatewayMessageWithSlippageArgs( | ||
| IERC20 tokenSent, | ||
| uint256 amountIn, | ||
| address recipient, | ||
| StrategySlippageArgs memory args | ||
| ) public override { | ||
| tokenSent.safeTransferFrom(_msgSender(), address(this), amountIn); | ||
| tokenSent.safeIncreaseAllowance(address(solvBTCRouterV2), amountIn); | ||
|
|
||
| // Expiry time must be in the future, so we use 1 second from now since the deposit will happen atomically | ||
| // deposit input token into SolvBTC | ||
| uint256 solvBTCAmount = solvBTCRouterV2.deposit( | ||
| address(solvBTC), address(tokenSent), amountIn, args.amountOutMin, uint64(block.timestamp + 1) | ||
| ); | ||
|
|
||
| solvBTC.safeIncreaseAllowance(address(solvBTCRouterV1), solvBTCAmount); | ||
|
|
||
| // now deposit SolvBTC for SolvBTC.JUP | ||
| uint256 solvBTCJUPAmount = solvBTCRouterV1.createSubscription(poolId, solvBTCAmount); | ||
|
|
||
| solvBTCJUP.safeTransfer(recipient, solvBTCJUPAmount); | ||
|
|
||
| emit TokenOutput(address(solvBTCJUP), solvBTCJUPAmount); | ||
| } | ||
| } | ||
|
|
||
| contract SolvBTCPlusStrategy is IStrategyWithSlippageArgs, Context { | ||
| using SafeERC20 for IERC20; | ||
|
|
||
| ISolvBTCRouterV2 public immutable solvBTCRouter; | ||
| IERC20 public immutable solvBTCPlus; | ||
|
|
||
| constructor(ISolvBTCRouterV2 _solvBTCRouter, IERC20 _solvBTCPlus) { | ||
| solvBTCRouter = _solvBTCRouter; | ||
| solvBTCPlus = _solvBTCPlus; | ||
| } | ||
|
|
||
| function handleGatewayMessageWithSlippageArgs( | ||
| IERC20 tokenSent, | ||
| uint256 amountIn, | ||
| address recipient, | ||
| StrategySlippageArgs memory args | ||
| ) public override { | ||
| tokenSent.safeTransferFrom(_msgSender(), address(this), amountIn); | ||
| tokenSent.safeIncreaseAllowance(address(solvBTCRouter), amountIn); | ||
|
|
||
| // Expiry time must be in the future, so we use 1 second from now since the deposit will happen atomically | ||
| uint256 solvBTCPlusAmount = solvBTCRouter.deposit( | ||
| address(solvBTCPlus), address(tokenSent), amountIn, args.amountOutMin, uint64(block.timestamp + 1) | ||
| ); | ||
|
|
||
| solvBTCPlus.safeTransfer(recipient, solvBTCPlusAmount); | ||
|
|
||
| emit TokenOutput(address(solvBTCPlus), solvBTCPlusAmount); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new strategies SolvBTCJUPStrategy and SolvBTCPlusStrategy handle token transfers and interactions with external contracts. There's a risk that some amount of tokens (e.g., dust from swaps or tokens sent to the contract by mistake) could get stuck. It's a good practice to add a sweep function to both contracts that allows an owner/admin to withdraw any arbitrary ERC20 token to prevent loss of funds. This would involve adding an owner, an onlyOwner modifier, and the sweep function itself.
| uint256 solvBTCAmount = solvBTCRouterV2.deposit( | ||
| address(solvBTC), address(tokenSent), amountIn, args.amountOutMin, uint64(block.timestamp + 1) | ||
| ); |
There was a problem hiding this comment.
Using a deadline of block.timestamp + 1 is very tight. This pattern is used in both SolvBTCJUPStrategy and SolvBTCPlusStrategy. While the comment mentions the deposit is atomic, if the transaction is not mined in the next second (e.g., due to network congestion), it will revert. It's safer to use a slightly longer deadline (e.g., a few minutes) to make the strategies more robust.
uint256 solvBTCAmount = solvBTCRouterV2.deposit(
address(solvBTC), address(tokenSent), amountIn, args.amountOutMin, uint64(block.timestamp + 180) // 3 minutes deadline
);
| abstract contract ForkedStrategyTemplateWbtcOft is Test { | ||
| IERC20 public token; | ||
|
|
||
| constructor() { | ||
| token = IERC20(Constants.WBTC_OFT_ADDRESS); | ||
| } | ||
|
|
||
| function simulateForkAndTransfer(uint256 forkAtBlock, address sender, address receiver, uint256 amount) public { | ||
| vm.createSelectFork(vm.envString("BOB_PROD_PUBLIC_RPC_URL"), forkAtBlock); | ||
| vm.prank(sender); | ||
| token.transfer(receiver, amount); | ||
| } | ||
| } |
There was a problem hiding this comment.
This new abstract contract ForkedStrategyTemplateWbtcOft is very similar to ForkedStrategyTemplateWbtc and ForkedStrategyTemplateTbtc. To avoid code duplication and improve maintainability, consider refactoring them into a single generic base template that accepts the token address as a constructor argument.
For example:
abstract contract ForkedStrategyTemplate is Test {
IERC20 public token;
constructor(address tokenAddress) {
token = IERC20(tokenAddress);
}
function simulateForkAndTransfer(uint256 forkAtBlock, address sender, address receiver, uint256 amount) public {
vm.createSelectFork(vm.envString("BOB_PROD_PUBLIC_RPC_URL"), forkAtBlock);
vm.prank(sender);
token.transfer(receiver, amount);
}
}| strategy.handleGatewayMessageWithSlippageArgs(token, 1e8, Constants.DUMMY_RECEIVER, StrategySlippageArgs(0)); | ||
| vm.stopPrank(); | ||
|
|
||
| assertApproxEqAbs(solvBTCJUP.balanceOf(Constants.DUMMY_RECEIVER), 1e18, 1e18 / 10); |
There was a problem hiding this comment.
The 10% delta (1e18 / 10) used in this assertion is quite large. This might be expected due to fees or slippage in the two-step swap process, but it could also hide potential issues. It would be beneficial to add a comment explaining why such a large deviation is expected. If it's not expected, this might need further investigation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
contracts/test/gateway/e2e-strategy-tests/ForkedTemplate.sol (1)
27-39: De-duplicate fork templates via a parameterized base.This repeats WBTC/TBTC logic; extract a common base and make this a 1-line child.
Apply within this hunk:
-abstract contract ForkedStrategyTemplateWbtcOft is Test { - IERC20 public token; - - constructor() { - token = IERC20(Constants.WBTC_OFT_ADDRESS); - } - - function simulateForkAndTransfer(uint256 forkAtBlock, address sender, address receiver, uint256 amount) public { - vm.createSelectFork(vm.envString("BOB_PROD_PUBLIC_RPC_URL"), forkAtBlock); - vm.prank(sender); - token.transfer(receiver, amount); - } -} +abstract contract ForkedStrategyTemplateWbtcOft is ForkedStrategyTemplate(Constants.WBTC_OFT_ADDRESS) {}Add this base (outside the hunk) to replace the repeated code:
abstract contract ForkedStrategyTemplate is Test { IERC20 public token; constructor(address tokenAddress) { token = IERC20(tokenAddress); } function simulateForkAndTransfer(uint256 forkAtBlock, address sender, address receiver, uint256 amount) public { vm.createSelectFork(vm.envString("BOB_PROD_PUBLIC_RPC_URL"), forkAtBlock); // Optional: precondition to avoid flaky forks with depleted EOAs require(token.balanceOf(sender) >= amount, "insufficient sender balance at fork"); vm.prank(sender); token.transfer(receiver, amount); } }Optionally also convert Wbtc/Tbtc children to this pattern for full DRY.
contracts/script/DeployStrategy.s.sol (2)
10-10: Remove or use the unused deployer variable.Set it or drop it.
Apply:
- address public deployer; + address public deployer; @@ - uint256 deployerPrivateKey = vm.envUint("PRIVATE_KEY"); + uint256 deployerPrivateKey = vm.envUint("PRIVATE_KEY"); + deployer = vm.addr(deployerPrivateKey);
23-23: Deploy SolvBTCPlusStrategy as well and surface deployed addresses.The PR adds both strategies; deploy both for parity and log addresses.
Apply:
- new SolvBTCJUPStrategy(solvBTCRouterV1, solvBTCRouterV2, poolId, solvBTC, solvBTCJUP); + SolvBTCJUPStrategy jup = new SolvBTCJUPStrategy(solvBTCRouterV1, solvBTCRouterV2, poolId, solvBTC, solvBTCJUP); + // Optional: Plus strategy deployment (needs SOLV_BTC_PLUS env or constant) + IERC20 solvBTCPlus = IERC20(vm.envAddress("SOLV_BTC_PLUS")); + SolvBTCPlusStrategy plus = new SolvBTCPlusStrategy(solvBTCRouterV2, solvBTCPlus); + // Surface addresses (can also use console2) + vm.label(address(jup), "SolvBTCJUPStrategy"); + vm.label(address(plus), "SolvBTCPlusStrategy");contracts/test/gateway/e2e-strategy-tests/SolvStrategyForked.sol (1)
58-74: 10% tolerance is high—justify or tighten; centralize addresses.
- If 10% deviation is expected (fees, 2-step route), add a brief comment with rationale; otherwise consider ≤3–5%.
- The router/token/poolId literals repeat across tests; consider local constants or a small helper to reduce duplication.
Example constants near the top of the test:
uint256 constant ONE_WBTC = 1e8; ISolvBTCRouterV2 constant ROUTER_V2 = ISolvBTCRouterV2(0x56a4d8...3B48); ISolvBTCRouterV1 constant ROUTER_V1 = ISolvBTCRouterV1(0xbA46Fc...5bA1); bytes32 constant JUP_POOL_ID = 0x6f113a...d1d04;contracts/src/gateway/strategy/SolvStrategy.sol (2)
126-204: Add an owner-admin sweep to prevent stuck funds (dust, mistaken sends)Echoing prior feedback: add an admin-controlled sweep for arbitrary ERC20 to recover dust.
Example (requires OZ Ownable):
// imports: import {Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol"; // change: contract SolvBTCJUPStrategy is IStrategyWithSlippageArgs, Context, Ownable2Step { ... } contract SolvBTCPlusStrategy is IStrategyWithSlippageArgs, Context, Ownable2Step { ... } // add to each: function sweep(IERC20 token, address to, uint256 amount) external onlyOwner { require(to != address(0), "to=0"); token.safeTransfer(to, amount); }
149-173: Harden approvals, extend deadline, and validate recipient
- Replace increaseAllowance with forceApprove to set exact spend.
- Clear allowances post-call to avoid residual spend if router under-consumes.
- Use a less brittle deadline.
- Guard against zero recipient.
function handleGatewayMessageWithSlippageArgs( IERC20 tokenSent, uint256 amountIn, address recipient, StrategySlippageArgs memory args ) public override { - tokenSent.safeTransferFrom(_msgSender(), address(this), amountIn); - tokenSent.safeIncreaseAllowance(address(solvBTCRouterV2), amountIn); + require(recipient != address(0), "recipient=0"); + tokenSent.safeTransferFrom(_msgSender(), address(this), amountIn); + tokenSent.forceApprove(address(solvBTCRouterV2), amountIn); - // Expiry time must be in the future, so we use 1 second from now since the deposit will happen atomically + // Allow a small buffer to avoid needless deadline reverts uint256 solvBTCAmount = solvBTCRouterV2.deposit( - address(solvBTC), address(tokenSent), amountIn, args.amountOutMin, uint64(block.timestamp + 1) + address(solvBTC), address(tokenSent), amountIn, args.amountOutMin, uint64(block.timestamp + 180) ); + // Clear allowance to minimize exposure if router under-consumed + tokenSent.forceApprove(address(solvBTCRouterV2), 0); - solvBTC.safeIncreaseAllowance(address(solvBTCRouterV1), solvBTCAmount); + solvBTC.forceApprove(address(solvBTCRouterV1), solvBTCAmount); // now deposit SolvBTC for SolvBTC.JUP uint256 solvBTCJUPAmount = solvBTCRouterV1.createSubscription(poolId, solvBTCAmount); + // Clear allowance + solvBTC.forceApprove(address(solvBTCRouterV1), 0);Additionally, there’s no slippage guard on the v1 leg. If v1 supports a min-out parameter or a “preview” quote, consider adding an args field (e.g., amountOutMinJUP) and enforcing it before/after createSubscription. Otherwise, please confirm v1 is price-protected at the router/pool level.
🧹 Nitpick comments (8)
contracts/test/gateway/e2e-strategy-tests/ForkedTemplate.sol (1)
34-38: Add a precondition to reduce fork-test flakiness.Check sender balance before transfer so CI fails fast if the funding EOA at that block changes.
Apply:
function simulateForkAndTransfer(uint256 forkAtBlock, address sender, address receiver, uint256 amount) public { vm.createSelectFork(vm.envString("BOB_PROD_PUBLIC_RPC_URL"), forkAtBlock); + require(token.balanceOf(sender) >= amount, "insufficient sender balance at fork"); vm.prank(sender); token.transfer(receiver, amount); }contracts/test/gateway/e2e-strategy-tests/BedrockStrategyForked.sol (1)
16-16: Verify fork block and funding EOA; consider a named constant for 1 WBTC.
- Ensure 0xa79a… has ≥1e8 WBTC-OFT at block 21,930,500 so transfer doesn’t flake.
- Optional: introduce ONE_WBTC = 1e8 to avoid magic numbers.
contracts/script/DeployStrategy.s.sol (2)
12-16: Harden script preconditions.Guard against missing/zero PRIVATE_KEY to fail early.
Apply:
function run() public { uint256 deployerPrivateKey = vm.envUint("PRIVATE_KEY"); + require(deployerPrivateKey != 0, "PRIVATE_KEY not set"); vm.startBroadcast(deployerPrivateKey);
17-23: Avoid hard-coded addresses in scripts; move to env or config.These addresses are chain-specific. Prefer env vars (with sane defaults) or a config library to switch per network.
Example:
ISolvBTCRouterV2 solvBTCRouterV2 = ISolvBTCRouterV2(vm.envAddress("SOLV_ROUTER_V2")); ISolvBTCRouterV1 solvBTCRouterV1 = ISolvBTCRouterV1(vm.envAddress("SOLV_ROUTER_V1")); IERC20 solvBTC = IERC20(vm.envAddress("SOLV_BTC")); IERC20 solvBTCJUP = IERC20(vm.envAddress("SOLV_BTC_JUP")); bytes32 poolId = vm.envBytes32("SOLV_JUP_POOL_ID");contracts/test/gateway/e2e-strategy-tests/SolvStrategyForked.sol (1)
76-88: Plus strategy test reads well; minor DRY suggestion.Addresses and amount literals are duplicated; extract shared constants (as above). Otherwise OK.
contracts/src/gateway/strategy/SolvStrategy.sol (3)
10-21: Clarify v1 router docs to match JUP usageThe comment implies paying WBTC and receiving SolvBTC, but in SolvBTCJUPStrategy you pay SolvBTC (from V2 leg) and receive SolvBTC.JUP. Please generalize the doc to “pool-defined payment token → pool-defined target token,” and confirm the v1 router indeed accepts SolvBTC for the JUP pool.
Apply:
- * Subscribe with payment currency (i.e. WBTC) and receive SolvBTC. + * Subscribe with the pool-defined payment currency (e.g., WBTC or SolvBTC) and + * receive the pool-defined target token (e.g., SolvBTC or SolvBTC.JUP). - * @param currencyAmount Amount of currency to be paid. + * @param currencyAmount Amount of payment currency to be paid. - * @return shareValue Amount of SolvBTC to be received after subscription. + * @return shareValue Amount of target token shares/units to be received.
166-168: Add slippage protection for the JUP subscription leg (if supported)Without a min-out, users can receive unexpectedly low SolvBTC.JUP during volatile moments. Prefer a min-out or pre-validated share value.
149-173: Optional: add nonReentrant and/or Pausable for defense-in-depthWhile stateful risk is low, guarding external router calls is cheap insurance; pausing helps in incidents.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
contracts/script/DeployStrategy.s.sol(1 hunks)contracts/src/gateway/strategy/SolvStrategy.sol(2 hunks)contracts/test/gateway/e2e-strategy-tests/BedrockStrategyForked.sol(1 hunks)contracts/test/gateway/e2e-strategy-tests/Constants.sol(1 hunks)contracts/test/gateway/e2e-strategy-tests/ForkedTemplate.sol(1 hunks)contracts/test/gateway/e2e-strategy-tests/SolvStrategyForked.sol(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (5)
contracts/test/gateway/e2e-strategy-tests/Constants.sol (1)
10-10: Annotate network, fork height & verify OFT contract checksum
- In Constants.sol, add a comment on which chain (e.g. Ethereum mainnet) and fork block this WBTC_OFT_ADDRESS applies to, preventing accidental cross-chain reuse.
- Confirm the address matches the EIP-55 checksum and indeed points to the LayerZero WBTC Omnichain Fungible Token (not the canonical ERC-20 WBTC).
contracts/test/gateway/e2e-strategy-tests/BedrockStrategyForked.sol (1)
9-14: Switch to WBTC-OFT template: LGTM.Inheritance and import align with the new OFT constant/template.
contracts/test/gateway/e2e-strategy-tests/SolvStrategyForked.sol (3)
9-16: Imports align with new strategy variants: LGTM.
19-24: Switch to OFT template: LGTM.
26-26: Confirm fork context and funding for sender EOA.Block 21,939,620 and EOA 0xa79a… should have sufficient WBTC-OFT to transfer 1e8; otherwise tests may flake.
| contract SolvBTCJUPStrategy is IStrategyWithSlippageArgs, Context { | ||
| using SafeERC20 for IERC20; | ||
|
|
||
| ISolvBTCRouterV1 public immutable solvBTCRouterV1; | ||
| ISolvBTCRouterV2 public immutable solvBTCRouterV2; | ||
| bytes32 public immutable poolId; | ||
| IERC20 public immutable solvBTC; | ||
| IERC20 public immutable solvBTCJUP; | ||
|
|
||
| constructor( | ||
| ISolvBTCRouterV1 _solvBTCRouterV1, | ||
| ISolvBTCRouterV2 _solvBTCRouterV2, | ||
| bytes32 _poolId, | ||
| IERC20 _solvBTC, | ||
| IERC20 _solvBTCJUP | ||
| ) { | ||
| solvBTCRouterV1 = _solvBTCRouterV1; | ||
| solvBTCRouterV2 = _solvBTCRouterV2; | ||
| poolId = _poolId; | ||
| solvBTC = _solvBTC; | ||
| solvBTCJUP = _solvBTCJUP; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add constructor invariants (non-zero addresses, non-zero poolId)
Prevent bricked deployments and ambiguous pool selection.
constructor(
ISolvBTCRouterV1 _solvBTCRouterV1,
ISolvBTCRouterV2 _solvBTCRouterV2,
bytes32 _poolId,
IERC20 _solvBTC,
IERC20 _solvBTCJUP
) {
+ require(address(_solvBTCRouterV1) != address(0) && address(_solvBTCRouterV2) != address(0), "router=0");
+ require(address(_solvBTC) != address(0) && address(_solvBTCJUP) != address(0), "token=0");
+ require(_poolId != bytes32(0), "poolId=0");
solvBTCRouterV1 = _solvBTCRouterV1;
solvBTCRouterV2 = _solvBTCRouterV2;
poolId = _poolId;
solvBTC = _solvBTC;
solvBTCJUP = _solvBTCJUP;
}📝 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.
| contract SolvBTCJUPStrategy is IStrategyWithSlippageArgs, Context { | |
| using SafeERC20 for IERC20; | |
| ISolvBTCRouterV1 public immutable solvBTCRouterV1; | |
| ISolvBTCRouterV2 public immutable solvBTCRouterV2; | |
| bytes32 public immutable poolId; | |
| IERC20 public immutable solvBTC; | |
| IERC20 public immutable solvBTCJUP; | |
| constructor( | |
| ISolvBTCRouterV1 _solvBTCRouterV1, | |
| ISolvBTCRouterV2 _solvBTCRouterV2, | |
| bytes32 _poolId, | |
| IERC20 _solvBTC, | |
| IERC20 _solvBTCJUP | |
| ) { | |
| solvBTCRouterV1 = _solvBTCRouterV1; | |
| solvBTCRouterV2 = _solvBTCRouterV2; | |
| poolId = _poolId; | |
| solvBTC = _solvBTC; | |
| solvBTCJUP = _solvBTCJUP; | |
| } | |
| contract SolvBTCJUPStrategy is IStrategyWithSlippageArgs, Context { | |
| using SafeERC20 for IERC20; | |
| ISolvBTCRouterV1 public immutable solvBTCRouterV1; | |
| ISolvBTCRouterV2 public immutable solvBTCRouterV2; | |
| bytes32 public immutable poolId; | |
| IERC20 public immutable solvBTC; | |
| IERC20 public immutable solvBTCJUP; | |
| constructor( | |
| ISolvBTCRouterV1 _solvBTCRouterV1, | |
| ISolvBTCRouterV2 _solvBTCRouterV2, | |
| bytes32 _poolId, | |
| IERC20 _solvBTC, | |
| IERC20 _solvBTCJUP | |
| ) { | |
| require(address(_solvBTCRouterV1) != address(0) && address(_solvBTCRouterV2) != address(0), "router=0"); | |
| require(address(_solvBTC) != address(0) && address(_solvBTCJUP) != address(0), "token=0"); | |
| require(_poolId != bytes32(0), "poolId=0"); | |
| solvBTCRouterV1 = _solvBTCRouterV1; | |
| solvBTCRouterV2 = _solvBTCRouterV2; | |
| poolId = _poolId; | |
| solvBTC = _solvBTC; | |
| solvBTCJUP = _solvBTCJUP; | |
| } | |
| } |
🤖 Prompt for AI Agents
In contracts/src/gateway/strategy/SolvStrategy.sol around lines 126 to 147, the
constructor lacks invariants allowing zero addresses or a zero poolId which can
brick deployments; add require checks asserting each router and token address is
not address(0) and poolId is not bytes32(0) with clear revert messages (e.g.,
"SolvStrategy: zero routerV1", "SolvStrategy: zero routerV2", "SolvStrategy:
zero poolId", "SolvStrategy: zero solvBTC", "SolvStrategy: zero solvBTCJUP")
before assigning state variables so invalid inputs are rejected immediately.
| contract SolvBTCPlusStrategy is IStrategyWithSlippageArgs, Context { | ||
| using SafeERC20 for IERC20; | ||
|
|
||
| ISolvBTCRouterV2 public immutable solvBTCRouter; | ||
| IERC20 public immutable solvBTCPlus; | ||
|
|
||
| constructor(ISolvBTCRouterV2 _solvBTCRouter, IERC20 _solvBTCPlus) { | ||
| solvBTCRouter = _solvBTCRouter; | ||
| solvBTCPlus = _solvBTCPlus; | ||
| } | ||
|
|
||
| function handleGatewayMessageWithSlippageArgs( | ||
| IERC20 tokenSent, | ||
| uint256 amountIn, | ||
| address recipient, | ||
| StrategySlippageArgs memory args | ||
| ) public override { | ||
| tokenSent.safeTransferFrom(_msgSender(), address(this), amountIn); | ||
| tokenSent.safeIncreaseAllowance(address(solvBTCRouter), amountIn); | ||
|
|
||
| // Expiry time must be in the future, so we use 1 second from now since the deposit will happen atomically | ||
| uint256 solvBTCPlusAmount = solvBTCRouter.deposit( | ||
| address(solvBTCPlus), address(tokenSent), amountIn, args.amountOutMin, uint64(block.timestamp + 1) | ||
| ); | ||
|
|
||
| solvBTCPlus.safeTransfer(recipient, solvBTCPlusAmount); | ||
|
|
||
| emit TokenOutput(address(solvBTCPlus), solvBTCPlusAmount); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror hardening in SolvBTCPlusStrategy (approvals, deadline, recipient)
Same concerns as JUP strategy: exact approvals, clear after, longer deadline, non-zero recipient.
function handleGatewayMessageWithSlippageArgs(
IERC20 tokenSent,
uint256 amountIn,
address recipient,
StrategySlippageArgs memory args
) public override {
- tokenSent.safeTransferFrom(_msgSender(), address(this), amountIn);
- tokenSent.safeIncreaseAllowance(address(solvBTCRouter), amountIn);
+ require(recipient != address(0), "recipient=0");
+ tokenSent.safeTransferFrom(_msgSender(), address(this), amountIn);
+ tokenSent.forceApprove(address(solvBTCRouter), amountIn);
- // Expiry time must be in the future, so we use 1 second from now since the deposit will happen atomically
uint256 solvBTCPlusAmount = solvBTCRouter.deposit(
- address(solvBTCPlus), address(tokenSent), amountIn, args.amountOutMin, uint64(block.timestamp + 1)
+ address(solvBTCPlus), address(tokenSent), amountIn, args.amountOutMin, uint64(block.timestamp + 180)
);
+ tokenSent.forceApprove(address(solvBTCRouter), 0);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
contracts/src/gateway/strategy/SolvStrategy.sol lines 175-204: the function uses
an open-ended allowance, a very short deadline, and doesn't validate recipient;
change to set allowance to zero then set exact approval for amountIn (use
safeApprove(0) then safeApprove(amountIn) or safeIncreaseAllowance after
zeroing), perform a require(recipient != address(0)) at the start, extend the
deadline to a reasonable buffer (e.g. uint64(block.timestamp + 300)) when
calling solvBTCRouter.deposit, and clear the allowance after the deposit
(safeApprove(address(solvBTCRouter), 0)) to avoid lingering approvals.
| constructor(ISolvBTCRouterV2 _solvBTCRouter, IERC20 _solvBTCPlus) { | ||
| solvBTCRouter = _solvBTCRouter; | ||
| solvBTCPlus = _solvBTCPlus; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constructor invariants for Plus strategy
Match the safeguards added to JUP strategy.
constructor(ISolvBTCRouterV2 _solvBTCRouter, IERC20 _solvBTCPlus) {
+ require(address(_solvBTCRouter) != address(0), "router=0");
+ require(address(_solvBTCPlus) != address(0), "token=0");
solvBTCRouter = _solvBTCRouter;
solvBTCPlus = _solvBTCPlus;
}📝 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.
| constructor(ISolvBTCRouterV2 _solvBTCRouter, IERC20 _solvBTCPlus) { | |
| solvBTCRouter = _solvBTCRouter; | |
| solvBTCPlus = _solvBTCPlus; | |
| } | |
| constructor(ISolvBTCRouterV2 _solvBTCRouter, IERC20 _solvBTCPlus) { | |
| require(address(_solvBTCRouter) != address(0), "router=0"); | |
| require(address(_solvBTCPlus) != address(0), "token=0"); | |
| solvBTCRouter = _solvBTCRouter; | |
| solvBTCPlus = _solvBTCPlus; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/gateway/strategy/SolvStrategy.sol around lines 181 to 184, the
constructor currently assigns solvBTCRouter and solvBTCPlus without any input
validation; add the same constructor invariants used in the JUP strategy by
requiring that the provided _solvBTCRouter and _solvBTCPlus addresses are
non-zero (e.g. require(address(_solvBTCRouter) != address(0) &&
address(_solvBTCPlus) != address(0), "Invalid constructor args")), then set the
state variables; optionally mark these variables immutable if they are not
modified elsewhere.
Adding SolvBTC.JUP and SolvBTCPlus strategies and testing those along with Bedrock and SolvBTC with the new WBTC OFT token
Summary by CodeRabbit
New Features
Tests