-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor getStrategyConfig to handle old positions with no debt #4022
Refactor getStrategyConfig to handle old positions with no debt #4022
Conversation
WalkthroughThe changes in this pull request introduce enhancements to the Aave user configuration logic, including case-insensitive asset name handling and improved management of legacy positions without debts. Additionally, a new button is added to the NotFoundPage component for easier navigation during development, along with a corresponding localization entry for user interface consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NotFoundPage
participant AppLink
User->>NotFoundPage: Access 404 Page
NotFoundPage->>User: Display Not Found Message
alt Not in Production
NotFoundPage->>AppLink: Show "Go back" Button
User->>AppLink: Click "Go back"
AppLink->>User: Navigate to Previous Page
end
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 (
|
There was a problem hiding this 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
Outside diff range and nitpick comments (1)
pages/not-found.tsx (1)
27-33
: Improve button styling and accessibility.The conditional rendering and button functionality look good! Here are a couple of suggestions to improve the code:
- Instead of using a magic number for the top margin, consider using a value from the theme-ui spacing scale for better consistency and maintainability. For example:
-<AppLink href="/" sx={{ mt: '-20px' }}> +<AppLink href="/" sx={{ mt: -3 }}>
- Update the button text to be more descriptive for better accessibility. For example:
-{t('404-back')} +{t('404-back-to-previous-page')}And update the translation file accordingly.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- features/aave/helpers/getStrategyConfig.ts (1 hunks)
- lendingProtocols/aave-v2/pipelines/aave-user-configuration.ts (3 hunks)
- pages/not-found.tsx (1 hunks)
- public/locales/en/common.json (1 hunks)
Additional comments not posted (5)
lendingProtocols/aave-v2/pipelines/aave-user-configuration.ts (3)
12-14
: LGTM!The modification to convert all asset names in the
reserveNamesDictionary
to lowercase is a good change. It ensures case-insensitive lookups and prevents potential mismatches due to case sensitivity when referencing asset names.
69-73
: LGTM!The update to use the lowercase version of the asset name when checking if a token exists in the
tokensDictionary
is a good change. It ensures case-insensitive lookups and maintains consistency with the modifications made to thereserveNamesDictionary
.
86-95
: LGTM!The refinement of the
hasAssets
function to include a special case for scenarios where there are no debt tokens is a good change. It introduces a more robust handling of user asset lists and ensures that the function can correctly identify asset presence even in edge cases where there is no associated debt.The implementation using a ternary operator to conditionally check the debt tokens only if the
debtToken
array is not empty is clean and effective.features/aave/helpers/getStrategyConfig.ts (1)
Line range hint
85-90
: LGTM!The new case statement correctly handles the special case for old positions with 'STETH' assets and no debt. The strategy is loaded with the appropriate parameters, and the case statement is placed in the correct order within the switch block. This change enhances the
getStrategyConfig$
function to accommodate a wider range of position scenarios.public/locales/en/common.json (1)
4-4
: LGTM!The addition of the
"404-back": "Go back"
key-value pair looks good. It adds a localized string for what I assume is a "Go back" button on a 404 error page, improving the user experience when encountering that error.
This pull request refactors the
getStrategyConfig
function to handle old positions with no debt. It includes changes to thegetStrategyConfig
function and thehasAssets
function. Additionally, a new button has been added to the 404 page for going back (dev only).Summary by CodeRabbit
New Features
Bug Fixes