Skip to content

Conversation

@rudewalt
Copy link
Contributor

@rudewalt rudewalt commented Aug 5, 2024

Summary by CodeRabbit

  • New Features

    • Introduced advanced decentralized finance capabilities to streamline token swaps, pool management, and asset operations.
    • Added new features for managing vault deposits/withdrawals and principal asset redemptions, with enhanced pricing data retrieval and exchange processing.
  • Tests

    • Expanded test coverage to validate various swap scenarios and token configurations, ensuring increased reliability.
  • Chores

    • Upgraded utility functions for improved gas monitoring, balance tracking, and integrated asynchronous delays.

@coderabbitai
Copy link

coderabbitai bot commented Aug 5, 2024

Walkthrough

This pull request introduces a new SpectraAdapter contract that implements token swapping and pool management functionalities within a decentralized finance context. It includes expanded interfaces for interacting with Curve pools, ERC4626 wrappers, and principal tokens. Additionally, the PR updates test suites, enums, and utility functions to support new swap scenarios, improved logging, and asynchronous operations.

Changes

File(s) Change Summary
packages/router/contracts/adapters/SpectraAdapter.sol New SpectraAdapter contract implementing IMarginlyAdapter and extending Ownable2Step; adds functions for swapExactInput, swapExactOutput, addPools, sweepDust, and incorporates custom error handling and pool management.
packages/router/contracts/adapters/interfaces/ICurvePool.sol
packages/router/contracts/adapters/interfaces/ISpectraErc4626Wrapper.sol
packages/router/contracts/adapters/interfaces/ISpectraPrincipalToken.sol
Added new public functions in ICurvePool (last_prices and an overloaded exchange) and introduced new interfaces (ISpectraErc4626Wrapper, ISpectraPrincipalToken) defining functions for vault share management, wrapping/unwrapping, maturity, redemption, and withdrawal.
packages/router/contracts/test/CurveEMATest/TestStableSwap2EMAOraclePool.sol Updated the exchange function to use uint256 parameters instead of int128 and added a new last_prices view function.
packages/router/test/int/SpectraAdapter.eth.spec.ts New comprehensive integration tests for SpectraAdapter covering multiple token configurations and swap scenarios (pre- and post-maturity).
packages/router/test/shared/tokens.ts Updated the EthereumMainnetERC20BalanceOfSlot enum with five new token entries.
packages/router/test/shared/utils.ts Updated Dex constants (PendleCurveRouter, PendleCurve) and added a new constant (Spectra); introduced new functions (showBalanceDelta, delay) and improved logging for gas usage and balance messages.

Sequence Diagram(s)

sequenceDiagram
    participant U as User
    participant SA as SpectraAdapter
    participant P as Pool
    U->>SA: Call swapExactInput(recipient, tokenIn, tokenOut, amountIn, minAmountOut, data)
    SA->>SA: Validate input and determine swap strategy (pre/post-maturity)
    SA->>P: Initiate token swap process
    P-->>SA: Return swapped token amount
    SA->>U: Return amountOut
Loading
sequenceDiagram
    participant O as Owner
    participant SA as SpectraAdapter
    participant DB as PoolData Storage
    O->>SA: Call addPools(poolsData)
    SA->>SA: Validate pool input and process data
    SA->>DB: Update getPoolData mapping with new pool parameters
Loading

Possibly related PRs

  • added pendle market adapter #184: Introduces pool management and token swapping functionalities similar to those in the SpectraAdapter contract.
  • pendleCurve adapter #203: Implements swap functions (swapExactInput and swapExactOutput) in the PendleCurveNgAdapter, closely related to SpectraAdapter’s swap logic.

Suggested reviewers

  • optifat

Poem

I'm a bunny with joyous code,
Hopping through contracts down the road,
Swaps and pools, a merry dance,
Each function call gives me a chance,
To celebrate with fluff and cheer—
CodeRabbit leaps, a bunny pioneer! 🐰✨

Tip

🌐 Web search-backed reviews and chat
  • We have enabled web search-based reviews and chat for all users. This feature allows CodeRabbit to access the latest documentation and information on the web.
  • You can disable this feature by setting web_search: false in the knowledge_base settings.
  • Please share any feedback in the Discord discussion.
✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@rudewalt
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Jan 27, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (8)
packages/router/contracts/adapters/SpectraAdapter.sol (4)

104-145: Consider defensive checks when wrapping/unwrapping tokens.

In _swapExactInputPreMaturity, you wrap or unwrap IBT using ISpectraErc4626Wrapper but assume that the wrap/unwrap will succeed. Consider verifying returned amounts to handle unexpected short/wrong token operations from third-party contracts.


363-406: Provide additional context on partial leftover tokens.

_swapExactOutputPostMaturity may leave dust tokens in the contract if extra amounts can’t be converted back to PT after maturity. Consider elaborating on these partial leftover scenarios and their upstream impact. You could add events or logs to signal leftover amounts for debugging.


432-452: Consider adding an event for successful swaps.

Functions like swapExactInput perform critical token transfers but do not emit any event. Emitting an event (e.g. SwapExactInputExecuted(...)) can facilitate on-chain monitoring and debugging.


481-485: Check for transfer failures in sweepDust.

sweepDust will transfer any leftover token to the owner. Although using safeTransfer, consider logging or reverting if dust is nonzero but transfer fails or is partial due to a noncompliant ERC20.

packages/router/contracts/adapters/interfaces/ICurvePool.sol (2)

50-51: Clarify the purpose of last_prices().

The dev comments mention that some pools have this method. For clarity, consider explaining in a natspec or doc comment how integrators should interpret or use this data, especially if it is optional or pool-specific.


52-62: Document the use_eth parameter in the new exchange function.

The exchange(uint256 i, uint256 j, uint256 dx, uint256 min_dy, bool use_eth, address receiver) function includes a use_eth parameter, but there is no mention of its usage in the interface comments. Provide documentation or usage notes to avoid confusion for implementers.

packages/router/contracts/test/CurveEMATest/TestStableSwap2EMAOraclePool.sol (1)

83-86: Redundant duplication in last_prices().

last_prices() simply returns the same price as last_price(). If both methods are required for compatibility, consider referencing one method from the other or documenting their differences for consistency.

packages/router/test/shared/utils.ts (1)

65-67: Consider adding input validation and documentation.

While the implementation is correct, consider these improvements:

  1. Add input validation for negative or non-numeric values
  2. Add JSDoc documentation explaining the purpose and usage
+/**
+ * Creates a Promise that resolves after the specified delay.
+ * @param ms The delay duration in milliseconds
+ * @throws {Error} If ms is negative or not a number
+ * @returns Promise that resolves after the delay
+ */
 export function delay(ms: number) {
+  if (typeof ms !== 'number' || ms < 0) {
+    throw new Error('Delay must be a non-negative number');
+  }
   return new Promise((resolve) => setTimeout(resolve, ms));
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1e82b60 and 9181349.

📒 Files selected for processing (8)
  • packages/router/contracts/adapters/SpectraAdapter.sol (1 hunks)
  • packages/router/contracts/adapters/interfaces/ICurvePool.sol (1 hunks)
  • packages/router/contracts/adapters/interfaces/ISpectraErc4626Wrapper.sol (1 hunks)
  • packages/router/contracts/adapters/interfaces/ISpectraPrincipalToken.sol (1 hunks)
  • packages/router/contracts/test/CurveEMATest/TestStableSwap2EMAOraclePool.sol (2 hunks)
  • packages/router/test/int/SpectraAdapter.eth.spec.ts (1 hunks)
  • packages/router/test/shared/tokens.ts (1 hunks)
  • packages/router/test/shared/utils.ts (3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/router/test/int/SpectraAdapter.eth.spec.ts

[error] 266-266: 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)

🪛 Gitleaks (8.21.2)
packages/router/test/int/SpectraAdapter.eth.spec.ts

65-65: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


70-70: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


112-112: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


117-117: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


159-159: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


164-164: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

🔇 Additional comments (18)
packages/router/contracts/adapters/SpectraAdapter.sol (3)

50-97: Validate coin indices more robustly in _addPools.

Currently, _addPools presumes that each curve pool has exactly two coins and does minimal validation. Consider adding extra checks (e.g., ensuring coins(0) and coins(1) are non-zero addresses and not duplicates) to reduce potential misconfigurations.


146-165: Ensure post-maturity logic is fully covered.

_swapExactInputPostMaturity reverts for any IBT-to-PT swap after maturity. Confirm you consistently handle all edge cases where users might attempt invalid conversions after maturity. Consider clarifying or documenting these constraints in the code or in a doc comment.


408-426: Double-check the usage of SafeERC20.forceApprove.

forceApprove is not standard in the official OpenZeppelin SafeERC20 library. Ensure this custom extension is well-tested, because misconfiguration of allowances can expose the contract to unexpected token spending.

packages/router/contracts/test/CurveEMATest/TestStableSwap2EMAOraclePool.sol (1)

34-48: Avoid casting uint256 to int128 without checks.

When converting uint256 i, j to int128(uint128(i)), ensure it safetly fits within the signed 128-bit range. Although tests may not exceed a large index, consider a safety check or explicit assumption so integrators know the expected range.

packages/router/test/shared/utils.ts (2)

53-53: LGTM! Improved string interpolation.

The change enhances readability by properly replacing the $symbol placeholder with the actual token symbol.


27-29: Verify the impact of Dex enum value changes.

The values for PendleCurveRouter and PendleCurve have been significantly changed (20→30, 21→31). While the changes maintain sequential ordering, please ensure these values aren't used in any stored data or test fixtures that might break due to these changes.

✅ Verification successful

The Dex enum value changes are safe to proceed with.

The codebase exclusively uses symbolic enum references (e.g., Dex.PendleCurve, Dex.PendleCurveRouter) rather than hardcoded numeric values, and the enum is only used in test utilities and test files. No stored or persisted values were found that could be impacted by this change.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any hardcoded usage of the old values (20, 21) in tests or fixtures
rg -l '(?:PendleCurveRouter|PendleCurve).*(?:20|21)' --type ts

Length of output: 64


Script:

#!/bin/bash
# Search for any usage of PendleCurveRouter and PendleCurve in tests
rg -A 3 -B 3 "(?:PendleCurveRouter|PendleCurve)" --type ts

# Also search for potential numeric usages in test files
fd "test" -e ts -x rg -A 3 -B 3 "(?:dex|Dex).*(?:20|21|30|31)"

Length of output: 71561

packages/router/test/shared/tokens.ts (2)

19-19: Helpful documentation addition!

The comment about finding slots in block explorer improves maintainability and helps other developers understand how to find storage slots.


26-29: LGTM! Token balance slots added correctly.

The new balance slots are properly formatted as hex strings and align with the test cases in SpectraAdapter.eth.spec.ts.

packages/router/contracts/adapters/interfaces/ISpectraPrincipalToken.sol (3)

4-9: Well-structured interface with clear maturity check.

The maturity() function provides a clean way to determine when PTs become redeemable.


11-35: Good slippage protection for IBT redemption.

The overloaded redeemForIBT functions provide flexibility with an optional minIbts parameter for slippage protection.


37-59: Comprehensive IBT withdrawal functionality.

The overloaded withdrawIBT functions mirror the redemption pattern with proper slippage protection via maxShares.

packages/router/contracts/adapters/interfaces/ISpectraErc4626Wrapper.sol (3)

4-12: Clean interface design with proper vault share management.

The interface follows ERC4626 patterns with a clear way to access the underlying vault share.


14-19: Well-implemented wrapping functions with slippage protection.

The overloaded wrap functions provide proper slippage protection via minShares parameter.

Also applies to: 21-35


36-44: Helpful preview functions for off-chain calculations.

The preview functions allow for accurate estimation of wrapping/unwrapping amounts before executing transactions.

packages/router/test/int/SpectraAdapter.eth.spec.ts (4)

19-59: Well-structured test case type definition.

The TestCase type provides a clear structure for defining test scenarios with both pre and post maturity parameters.


204-264: Robust test initialization with proper balance checks.

The initializeRouter function properly sets up the test environment and validates initial balances.


274-362: Comprehensive pre-maturity swap testing.

All swap scenarios (exact input/output for both directions) are thoroughly tested with proper balance checks.


364-455: Complete post-maturity swap testing with proper restrictions.

The test suite properly validates that:

  • Pre-maturity swaps are forbidden after maturity
  • Post-maturity swaps work as expected

Comment on lines 167 to 265
function _swapExactOutputPtToIbtPreMaturity(
PoolData memory poolData,
address recipient,
address ibtOut,
uint256 ibtAmountOut,
uint256 maxPtAmountIn
) private returns (uint256 ptAmountIn) {
uint256 tokenInIndex = poolData.zeroIndexCoinIsIbt ? 1 : 0;

if (poolData.isSpectraWrappedIbt) {
// PT -> sw-IBT -> IBT
// convert ibAmountOut to swAmountOut
uint256 swAmountOut = ISpectraErc4626Wrapper(poolData.ibt).previewWrap(ibtAmountOut);

// swap maxAmountPt to swIbt
uint256 swActualAmountOut = _curveSwapExactInput(
poolData.pool,
address(this),
poolData.pt,
tokenInIndex,
maxPtAmountIn,
swAmountOut
);

// unwrap swIbt to ibt
uint256 ibtActualAmountOut = ISpectraErc4626Wrapper(poolData.ibt).unwrap(
swActualAmountOut,
address(this),
address(this)
);

IERC20(ibtOut).safeTransfer(recipient, ibtAmountOut);

if (ibtActualAmountOut < ibtAmountOut) {
revert TooMuchRequested();
}

if (ibtActualAmountOut == ibtAmountOut) {
return maxPtAmountIn;
}

uint256 deltaIbtAmountOut = ibtActualAmountOut - ibtAmountOut;

// wrap extra amountOut ibt to swIbt
IERC20(ibtOut).forceApprove(poolData.ibt, deltaIbtAmountOut);
uint256 deltaSwAmountOut = ISpectraErc4626Wrapper(poolData.ibt).wrap(deltaIbtAmountOut, address(this));

// swap and move excessive tokenIn directly to recipient.
// last arg minAmountOut is zero because we made worst allowed by user swap
// and an additional swap with whichever output only improves it,
// so the tx shouldn't be reverted
uint256 excessivePtAmountIn = _curveSwapExactInput(
poolData.pool,
recipient,
poolData.ibt,
1 - tokenInIndex,
deltaSwAmountOut,
0
);

ptAmountIn = maxPtAmountIn - excessivePtAmountIn;
} else {
// PT -> IBT
// swap maxAmountPt to ibt
uint256 ibtActualAmountOut = _curveSwapExactInput(
poolData.pool,
address(this),
poolData.pt,
tokenInIndex,
maxPtAmountIn,
ibtAmountOut
);

IERC20(ibtOut).safeTransfer(recipient, ibtAmountOut);

if (ibtActualAmountOut < ibtAmountOut) {
revert TooMuchRequested();
}

if (ibtActualAmountOut == ibtAmountOut) {
return maxPtAmountIn;
}

// swap and move excessive tokenIn directly to recipient.
// last arg minAmountOut is zero because we made worst allowed by user swap
// and an additional swap with whichever output only improves it,
// so the tx shouldn't be reverted
uint256 excessivePtAmountIn = _curveSwapExactInput(
poolData.pool,
recipient,
poolData.ibt,
1 - tokenInIndex,
ibtActualAmountOut - ibtAmountOut,
0
);

ptAmountIn = maxPtAmountIn - excessivePtAmountIn;
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor large swap functions for clarity.

Functions _swapExactOutputPtToIbtPreMaturity and _swapExactOutputIbtToPtPreMaturity contain multiple nested swaps and steps (wrap, unwrap, partial re-swaps). Refactoring them into smaller helper functions would improve readability and maintainability.

Comment on lines +17 to +19
error WrongPoolInput();
error UnknownPair();

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Declare all custom errors used in the code.

You are using custom errors like NotSupported(), TooMuchRequested(), and InsufficientAmount() throughout the contract but have only declared WrongPoolInput() and UnknownPair(). Declare all custom errors to avoid revert statements with undeclared errors.

Example fix:

+ error NotSupported();
+ error TooMuchRequested();
+ error InsufficientAmount();

error WrongPoolInput();
error UnknownPair();
📝 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.

Suggested change
error WrongPoolInput();
error UnknownPair();
error NotSupported();
error TooMuchRequested();
error InsufficientAmount();
error WrongPoolInput();
error UnknownPair();

};
}

describe.only('SpectraAdapter', async () => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove .only from the test suite.

The .only modifier will prevent other test suites from running. This should be removed before merging.

-describe.only('SpectraAdapter', async () => {
+describe('SpectraAdapter', 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.

Suggested change
describe.only('SpectraAdapter', async () => {
describe('SpectraAdapter', async () => {
🧰 Tools
🪛 Biome (1.9.4)

[error] 266-266: 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)

@rudewalt
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Jan 28, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
packages/router/test/int/SpectraAdapter.eth.spec.ts (2)

100-105: Remove or implement the commented-out swIbt block.

It appears that the swIbt configuration is partially defined but commented out. Either remove it to reduce clutter or fully implement it to ensure the test coverage is comprehensive.

-  // swIbt: {
-  //   address: '0xd89fc47aacbb31e2bf23ec599f593a4876d8c18c',
-  //   symbol: 'sw-inwstETHs',
-  //   ibtTransferAmount: parseUnits('500', 18),
-  // },
+  swIbt: {
+    address: '0xd89fc47aacbb31e2bf23ec599f593a4876d8c18c',
+    symbol: 'sw-inwstETHs',
+    ibtTransferAmount: parseUnits('500', 18),
+  },

425-425: Rename "Pendle swap post maturity" to "Spectra swap post maturity".

This suite of tests is for the Spectra adapter, so "Pendle swap post maturity" is confusing. Consider keeping naming consistent to avoid confusion.

-describe('Pendle swap post maturity', () => {
+describe('Spectra swap post maturity', () => {
packages/router/test/shared/utils.ts (1)

78-80: Helpful delay function.

The delay utility is useful for asynchronous scenarios, but ensure it does not introduce test flakiness or unreasonably long test runs if used too frequently or with large intervals.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9181349 and 857cc78.

📒 Files selected for processing (4)
  • packages/router/contracts/adapters/SpectraAdapter.sol (1 hunks)
  • packages/router/test/int/SpectraAdapter.eth.spec.ts (1 hunks)
  • packages/router/test/shared/tokens.ts (1 hunks)
  • packages/router/test/shared/utils.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/router/test/shared/tokens.ts
  • packages/router/contracts/adapters/SpectraAdapter.sol
🔇 Additional comments (4)
packages/router/test/int/SpectraAdapter.eth.spec.ts (1)

316-316: Comprehensive test coverage.

The overall structure of the test suite is well-designed and provides broad coverage for multiple token setups and maturity conditions.

packages/router/test/shared/utils.ts (3)

53-53: Clearer console output.

Replacing $symbol with the actual token symbol in log messages greatly improves readability and reduces confusion.


57-68: Useful utility for balance deltas.

The showBalanceDelta function is a neat addition to track net changes in token balances, enhancing test clarity.


27-29: Validate Dex index ordering.

Adding new entries and shifting existing ones works here, but ensure this ordering doesn’t break any assumptions in the codebase about consecutive indices or backward compatibility.

✅ Verification successful

Dex index ordering validation successful

The new Dex indices are safely appended at the end and used only for identification purposes. No assumptions about consecutive indices or dependencies on index ordering were found in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify Dex index usage across the repository.

rg -A 5 "Dex\." 

Length of output: 86623

@rudewalt rudewalt changed the title Draft: Feature/spectra adapter Feature/spectra adapter Jan 29, 2025
@rudewalt
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Jan 31, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

amountOut = IERC4626(poolData.ibt).redeem(ibtAmountOut, recipient, address(this));
} else {
amountOut = ISpectraPrincipalToken(poolData.pt).redeemForIBT(amountIn, recipient, address(this));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see, this code is logically similar to the PreMaturity one:

if (tokenInIsPt) {
    address recipient = recipientArg;

    if (poolData.quoteTokenKind != QuoteTokenKind.Ibt) {
        recipient = address(this);
    }
    amountOut = _curveSwapExactInput(...) / redeemForIBT(...);

    if (poolData.quoteTokenKind == QuoteTokenKind.IbtNotCompatibleWithERC4626) {
        // sw-IBT unwrap to IBT
        amountOut = ISpectraErc4626Wrapper(poolData.ibt).unwrap(amountOut, recipientArg, address(this));
    } else if (poolData.quoteTokenKind == QuoteTokenKind.UnderlyingOfIbt) {
        // IBT redeem to underlying asset
        amountOut = IERC4626(poolData.ibt).redeem(amountOut, recipientArg, address(this));
    }
} else {
    if(_ptIsExpired(pt)) revert NotSupported();
    
    **else from `_swapExactInputPreMaturity`**
}

maybe it makes sense to join current methods in swapExactInput and create a split like

function _swapExactPtInToIb(...) {
    if(_ptIsExpired(pt)) { 
        redeemForIBT(...);
    } else { 
        _curveSwapExactInput(...);
    }
}` 

@rudewalt
Copy link
Contributor Author

rudewalt commented Feb 4, 2025

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Feb 4, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (5)
packages/router/contracts/adapters/interfaces/ISpectraPrincipalToken.sol (1)

19-35: Consider emitting events for redemption and withdrawal.

The interface defines multiple functions (redeemForIBT, withdrawIBT, redeem, withdraw) that burn shares in exchange for IBTs or assets. To track these actions in the implementation, consider emitting events to enhance transparency and improve auditability.

+/// @notice Emitted upon successful redemption or withdrawal of IBTs or assets
+event Redemption(address indexed owner, uint256 sharesBurnt, uint256 ibtsOrAssetsReceived);

/// Example usage inside implementation (not in interface):
- function redeemForIBT(...) external returns (uint256 ibts) {
-   ...
-   // Implementation here
- }
+ function redeemForIBT(...) external returns (uint256 ibts) {
+   ...
+   emit Redemption(owner, shares, ibts);
+ }

Also applies to: 44-59, 69-79

packages/router/test/int/SpectraAdapter.eth.spec.ts (4)

351-421: Initialize router method is well-structured.

The initializeRouter function properly configures the user’s token balances and deployments. Consider verifying final pool states (like allowance or approvals) if needed for more robust setup checks.


423-531: Testing pre-maturity swaps thoroughly.

The pre-maturity tests cover exact input/output swaps for both directions (PT↔IBT). This is excellent for detecting regressions. Additionally, you might consider edge-case testing with zero or near-zero inputs to confirm revert scenarios.


533-630: Clear coverage for post-maturity restrictions.

Prohibiting certain swaps post-maturity is validated here. Good use of evm_increaseTime to simulate expiry. Consider adding a scenario where partial swap is attempted after maturity to ensure correct reverts.


633-634: Revisit forcing a 3-second delay.

A manual delay might slow down automated test runs unnecessarily. If this delay simulates real-world conditions, consider an alternative approach (e.g., evm_increaseTime). Otherwise, removing or shortening it could speed up test execution.

-    await delay(3000);
+    // Remove unnecessary delay or replace with an evm_increaseTime if time shift is needed
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c9359e and c9e71fa.

📒 Files selected for processing (3)
  • packages/router/contracts/adapters/SpectraAdapter.sol (1 hunks)
  • packages/router/contracts/adapters/interfaces/ISpectraPrincipalToken.sol (1 hunks)
  • packages/router/test/int/SpectraAdapter.eth.spec.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/router/contracts/adapters/SpectraAdapter.sol
🔇 Additional comments (2)
packages/router/contracts/adapters/interfaces/ISpectraPrincipalToken.sol (1)

4-9: Interface naming and doc clarity look good.

The naming convention and doc comments for maturity() provide clear insights. This function is straightforward, and no immediate changes are needed.

packages/router/test/int/SpectraAdapter.eth.spec.ts (1)

1-25: Comprehensive test setup and coverage.

Import statements and utility functions (e.g., resetFork, constructSwap) are utilized effectively. The test scaffolding for multiple token scenarios appears thorough.

@rudewalt rudewalt merged commit 6c08f33 into feature/marginly-pendle Feb 10, 2025
3 checks passed
@rudewalt rudewalt deleted the feature/spectra-adapter branch February 10, 2025 14:31
@coderabbitai coderabbitai bot mentioned this pull request Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants