Feature/pendle pt to asset adapter #207
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a new adapter type for handling Pendle Principal Token to Asset swaps. A new interface and corresponding types have been added to the deployment configuration files. The deployment flow now supports the Pendle adapter with added type guards and parameter creation methods. Additionally, a new smart contract implementing the swap functionality is introduced along with comprehensive tests and an update to the Dex enumeration. Changes
Sequence Diagram(s)sequenceDiagram
participant D as MarginlyRouterDeployer
participant T as Adapter Type Guard
participant C as createAdapterParam
participant A as Adapter Contract
D->>T: Check if adapter is PendlePtToAsset
T-->>D: Return true/false
alt PendlePtToAsset adapter
D->>C: Build adapter parameters using PendlePtToAssetAdapterParam
C-->>D: Return args array
D->>A: Deploy adapter with Pendle-specific args
else Other adapter
D->>A: Deploy using existing adapter logic
end
sequenceDiagram
participant U as User
participant A as PendlePtToAssetAdapter
participant M as Market Data Handler
U->>A: Call swapExactInput/swapExactOutput
A->>M: Validate pool and fetch market data
A->>A: Execute pre-/post-maturity swap logic
A->>U: Return swap result (amount in/out)
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![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 (6)
packages/router/contracts/adapters/PendlePtToAssetAdapter.sol (3)
20-28: Consider clarifying theslippagedata type to avoid confusion.Storing slippage as a
uint8and restricting its value to< 100(viainput.slippage >= ONE) is a practical approach, but it may be helpful to explicitly document that this represents a percentage-based slippage in the range 0–99. This can improve clarity for future maintainers.
127-133: Improve error context in_getMarketDataSafe.When reverting with
UnknownPair(), consider appending more context (e.g., the token addresses) to aid debugging. This can save time when troubleshooting misconfigurations.- if (address(marketData.market) == address(0)) revert UnknownPair(); + if (address(marketData.market) == address(0)) { + revert UnknownPair(); // or revert UnknownPair(tokenA, tokenB); + }
322-349: Consider minimizing leftover tokens in_pendleMintSy.The comment notes that a small amount of SY may remain in the contract. You may want to track or emit an event for leftover tokens to simplify asset reconciliation. Alternatively, you can design logic (if feasible) to handle micro remainders once the swap is completed.
packages/deploy/src/deployer/configs.ts (2)
226-232:PendlePtToAssetAdapterParaminterface definition is clear.All necessary fields (
pendleMarket,ptToken,assetToken,slippage) are well-defined. Consider a docstring explaining thatslippageis in a percentage-like format (0–99).
673-692: Use explicit class references increateAdapterParamto avoidthisusage in a static context.The static analysis tools warn about using
thisin a static method. Replacethiswith the class name (e.g.,StrictMarginlyDeployConfig) for clarity and to conform to the recommended practice.- const adapterParam = this.createAdapterParam(adapter.adapterName, pool, tokens, dexId); + const adapterParam = StrictMarginlyDeployConfig.createAdapterParam(adapter.adapterName, pool, tokens, dexId);🧰 Tools
🪛 Biome (1.9.4)
[error] 680-680: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 682-682: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 684-684: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 686-686: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 688-688: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 690-690: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
packages/router/test/int/PendlePtToAsset.eth.spec.ts (1)
266-271: Consider making slippage configurable.The slippage value is hardcoded to 35. Consider making it configurable through the test case configuration for better flexibility.
Apply this diff:
const poolInput: PendlePtToAssetAdapter.PoolInputStruct = { ptToken: ptToken.address, asset: assetToken.address, pendleMarket: testCase.pendleMarket, - slippage: 35, + slippage: testCase.slippage || 35, // Default to 35 if not specified };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/deploy/src/config.ts(2 hunks)packages/deploy/src/deployer/MarginlyRouterDeployer.ts(2 hunks)packages/deploy/src/deployer/configs.ts(7 hunks)packages/router/contracts/adapters/PendlePtToAssetAdapter.sol(1 hunks)packages/router/test/int/PendlePtToAsset.eth.spec.ts(1 hunks)packages/router/test/shared/utils.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/deploy/src/deployer/configs.ts
[error] 619-619: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 680-680: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 682-682: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 684-684: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 686-686: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 688-688: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 690-690: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
packages/router/test/int/PendlePtToAsset.eth.spec.ts
[error] 316-316: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🔇 Additional comments (12)
packages/router/contracts/adapters/PendlePtToAssetAdapter.sol (2)
108-126: Evaluate safeguards against re-entrancy inswapCallback.The contract requires
msg.sender == marketData.market, which is a good start. However, since this callback triggers external calls (e.g.,IMarginlyRouter(data.router).adapterCallback), it may benefit from having a stronger re-entrancy guard in place if there is any risk that these external calls could re-enter this contract unexpectedly.
371-406: Validate asset compatibility more thoroughly in_addPools.Currently, the checks for
ptToken,asset, andpendleMarketreferences and whethersy.isValidTokenIn(...)are good. However, consider verifying that the sameassetandptdo not already map to a differentPendleMarketData. Otherwise, repeated calls toaddPoolscould unintentionally override existing pairs.packages/deploy/src/deployer/configs.ts (4)
4-26: New adapter pair integration looks good.The addition of
PendlePtToAssetAdapterPairextends the existing adapters effectively. The approach is consistent with other adapter pair definitions.
178-179: ExtendedAdapterParamunion.Including
PendlePtToAssetAdapterParamhere is consistent with the other adapter parameter types. No issues spotted.
242-244: Helper guard function is consistent.
isPendlePtToAssetAdapteraligns well with the type-guard naming conventions used throughout this codebase.
761-784:createPendlePtToAssetAdapterParamlogic aligns well with adapter creation patterns.The method properly checks for missing tokens and slippage. The usage of
EthAddress.parse(...)is consistent. No further issues spotted.packages/router/test/shared/utils.ts (1)
30-30: NewDexentry forPendlePtToAssetis consistent.Adding
PendlePtToAsset: 33follows the established enumeration pattern. There are no apparent issues with referencing or usage of this newDexconstant.packages/deploy/src/deployer/MarginlyRouterDeployer.ts (2)
13-13: LGTM! New adapter types are properly imported.The new adapter type
PendlePtToAssetAdapterParamand its type guardisPendlePtToAssetAdapterare correctly imported, maintaining consistency with other adapter types.Also applies to: 18-18
65-76: LGTM! New adapter deployment logic follows established patterns.The deployment logic for
PendlePtToAssetAdapteris well-structured and follows the same pattern as other adapters, correctly handling the required parameters:
- pendleMarket
- slippage
- ptToken
- assetToken
packages/deploy/src/config.ts (2)
387-388: LGTM! AdapterPair type correctly updated.The
AdapterPairtype is properly extended to include the newPendlePtToAssetAdapterPairinterface.
432-437: LGTM! New adapter interface is well-defined.The
PendlePtToAssetAdapterPairinterface is properly defined with all necessary properties:
- tokenAId: string
- tokenBId: string
- pendleMarket: string
- slippage: number
packages/router/test/int/PendlePtToAsset.eth.spec.ts (1)
315-541: LGTM! Comprehensive test coverage.The test suite is well-structured with thorough coverage:
- Pre-maturity tests for exact input/output swaps
- Post-maturity tests including forbidden operations
- Multiple test cases with different tokens
- Proper balance checks and gas usage monitoring
🧰 Tools
🪛 Biome (1.9.4)
[error] 316-316: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
| } | ||
|
|
||
| // Tests for running in ethereum mainnet fork | ||
| describe.only('PendlePtToAssetAdapter', async () => { |
There was a problem hiding this comment.
Remove .only to enable all tests.
The .only modifier is typically used during development for debugging purposes and should be removed before merging.
Apply this diff:
-describe.only('PendlePtToAssetAdapter', async () => {
+describe('PendlePtToAssetAdapter', async () => {📝 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.
| describe.only('PendlePtToAssetAdapter', async () => { | |
| describe('PendlePtToAssetAdapter', async () => { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 316-316: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
| } else { | ||
| // this clause is realized when pt tokens is output | ||
| // we need to mint SY from Asset and send to pendle | ||
| _pendleMintSy(marketData, msg.sender, uint256(-syToAccount), data); |
There was a problem hiding this comment.
pt out -> exactInput -> swapCallback -> _pendleMintSy scenario causes double sy.previewDeposit call
I don't think this call is too gas pricey, however I'd research a possibility to place it into CallbackData struct
There was a problem hiding this comment.
I am not sure it would be significant reducing
There was a problem hiding this comment.
Yeah, I thought about that, moreover it'll make other calls worse because of more unnecessary memory allocations in those case
However I think on average the gas consumption will reduce (because the mentioned scenario seems to be common enough, occurring in deposit + flip), but the decrease should be insignificant enough
| ); | ||
| amountIn = ptAmountIn; | ||
| // use amountOut here, because actualSyAmountOut a little bit more than amountOut | ||
| _pendleRedeemSy(marketData, address(this), actualSyAmountOut); |
There was a problem hiding this comment.
why not to redeem estimatedSyOut amount to recipient and save gas on one additional transfer?
There was a problem hiding this comment.
However this change may cause those rounding issues with that (PENDLE_ONE * assetAmount + assetsPerSyUnit - 1) / assetsPerSyUnit line
| } | ||
|
|
||
| ///@dev Calc how much asset need to deposit and get exact amount of SY | ||
| function _syToAssetUpForDeposit(PendleMarketData memory marketData, uint256 syAmount) private view returns (uint256) { |
There was a problem hiding this comment.
this and _assetToSyUpForRedeem methods should be moved somewhere lower (to _redeemPY?) to at least preserve public methods -> swap implementation order (right now they are in between _swapExactInputPostMaturity and _swapExactOutputPostMaturity)
| uint256 index = marketData.yt.pyIndexCurrent(); | ||
| amountIn = Math.mulDiv(estimatedSyAmountOut, index, PENDLE_ONE, Math.Rounding.Up); | ||
| uint256 syAmountOut = _redeemPY(marketData.yt, msg.sender, amountIn, data); | ||
| _pendleRedeemSy(marketData, address(this), syAmountOut); |
There was a problem hiding this comment.
similar to the previous comment: why not to redeem estimatedSyAmountOut?
Summary by CodeRabbit
New Features
Refactor
Tests