Sky changes #4028
Sky changes #4028
Conversation
- SRR: icons fix
|
Warning Rate limit exceeded@marcinciarka has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 26 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes involve modifications across several files, primarily focusing on the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (6)
blockchain/better-calls/sky.ts (1)
Line range hint
118-133: Consider documenting the separation of concerns between stake detail functions.The changes to
skyUsdsWalletStakeDetailsand the unchangedskyUsdsStakeDetailsfunction suggest a clear separation of concerns:
skyUsdsWalletStakeDetails: Focuses on user-specific data (balance and earned rewards).skyUsdsStakeDetails: Handles global staking data (reward rate and total locked USDS).This separation aligns well with the PR objectives of improving UX for different view types (owner vs. non-owner).
Consider adding comments to both functions to clarify their specific purposes and usage scenarios. This will help maintain the clear separation of concerns in future development. For example:
/** * Retrieves user-specific staking details. * Use this for personalized wallet views. */ export const skyUsdsWalletStakeDetails = async ({ ownerAddress }: { ownerAddress?: string }) => { // ... (existing implementation) } /** * Retrieves global staking details. * Use this for general staking information views. */ export const skyUsdsStakeDetails = async () => { // ... (existing implementation) }Also applies to: 136-155
features/sky/components/SwapCard.tsx (1)
130-135: Approve changes with a minor suggestion.The introduction of the
showAllowanceInfoconstant improves code readability and reduces duplication by centralizing the allowance check logic. This is a good refactoring step.Consider renaming
showAllowanceInfoto something more specific likeshowAllowanceWarningorrequiresAllowanceUpdateto better reflect its purpose. This would make the intent clearer to other developers.helpers/context/ProductContext.ts (2)
692-693: LGTM: New observable function added correctlyThe new
skyUsdsStakeDetails$function has been implemented correctly, following the same pattern as the existingskyUsdsWalletStakeDetails$function. It will update on every new block, which is consistent with other observables in this context.For consistency, consider adding a comment explaining the purpose of this new function, similar to how other functions in this file are documented.
3-3: Summary: Sky USDS stake details functionality added successfullyThe changes in this file successfully introduce a new observable
skyUsdsStakeDetails$for fetching Sky USDS stake details. The modifications are well-integrated with the existing codebase, following established patterns and conventions. These changes enhance the product context by providing access to Sky USDS staking information, which can now be used in other parts of the application.As the product context grows with new observables and functions, consider grouping related functionality (e.g., all Sky USDS related observables) into separate modules or services to maintain code organization and scalability.
Also applies to: 692-693, 737-737
pages/[networkOrProduct]/sky/upgrade.tsx (1)
41-41: Include wallet account address in dependency arrayInside the
useMemo, you're usingwallet?.accounts[0].address, but the dependency array only includeswallet?.accounts. For proper memoization, includewallet?.accounts?.[0]?.addressin the dependency array.Apply this diff:
-[balancesFromAddressInfoArray$, wallet?.accounts], +[balancesFromAddressInfoArray$, wallet?.accounts?.[0]?.address],pages/earn/srr/[wallet].tsx (1)
50-55: OptimizeuseObservableHook forskyStakeDataThe
useObservablehook forskyStakeDataseems to be correctly set up. However, consider if any additional dependencies should be included in theuseMemodependency array to prevent unnecessary re-renders.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- blockchain/better-calls/sky.ts (2 hunks)
- features/sky/components/SwapCard.tsx (2 hunks)
- features/sky/components/stake/SkyStakePositionView.tsx (11 hunks)
- features/sky/hooks/useSky.ts (7 hunks)
- helpers/context/ProductContext.ts (3 hunks)
- pages/[networkOrProduct]/sky/upgrade.tsx (3 hunks)
- pages/earn/srr/[wallet].tsx (4 hunks)
- pages/earn/srr/index.tsx (2 hunks)
- theme/icons/oeth.tsx (2 hunks)
- theme/icons/sky.tsx (12 hunks)
- theme/icons/susds.tsx (2 hunks)
Files skipped from review due to trivial changes (2)
- theme/icons/oeth.tsx
- theme/icons/sky.tsx
Additional context used
Biome
pages/[networkOrProduct]/sky/upgrade.tsx
[error] 78-78: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Additional comments not posted (42)
pages/earn/srr/index.tsx (3)
1-7: LGTM: Import changes align with component updatesThe import changes are consistent with the simplification of the
SkyProxyPagecomponent. The removal of unused imports and the addition of theSkyStakeUsdsViewWrapperimport improve code cleanliness and reflect the new component structure.
Line range hint
8-16: LGTM:getServerSidePropsfunction remains validThe
getServerSidePropsfunction has not been modified. It continues to provide necessary server-side props, including translations and thewalletAddress. This is consistent with the component's needs and the overall page functionality.
Line range hint
18-24: Simplification ofSkyProxyPagecomponent needs clarificationThe
SkyProxyPagecomponent has been significantly simplified, which aligns with the PR objectives of enhancing wallet handling. However, there are a few points that need clarification:
The
AppLayoutandWithWalletConnectioncomponents have been removed. Could you confirm if these responsibilities have been moved elsewhere in the application?The component still uses
useAccountanduseRouterhooks, but their usage in the simplified component is not clear. Are these still necessary?The logic for replacing the URL when
walletAddressis not provided butwalletContextAddressis available remains unchanged. Is this still required given the new component structure?To verify the usage of
useAccountanduseRouterhooks, please run the following script:This will help us understand if these hooks are still necessary in the new component structure.
theme/icons/susds.tsx (4)
5-5: LGTM: Correct attribute naming for React JSXThe change from
clip-pathtoclipPathis correct and necessary for React JSX syntax. This modification prevents warnings and ensures proper rendering of the SVG in React components.
24-24: LGTM: Consistent attribute naming for React JSXThe change from
stop-colortostopColoris correct and consistent with React JSX syntax. This modification maintains the pattern of using camelCase for attribute names in React components.
25-25: LGTM: Consistent attribute naming and preserved functionalityThe change from
stop-colortostopColoris correct and consistent with the previous modifications. These changes collectively ensure that all SVG attributes in this file now use the correct camelCase naming convention required by React JSX.The functionality and appearance of the SVG remain unchanged, as only the attribute names were modified. These updates will resolve any warnings related to attribute naming and improve the overall code quality.
Line range hint
5-25: Summary: Successful resolution of icon warningsThe changes made to this file successfully address the icon warnings mentioned in the PR objectives. By updating the SVG attribute names from kebab-case to camelCase, the code now adheres to React JSX syntax requirements. This modification improves code quality and ensures proper rendering of the SVG icon without any warnings.
Key points:
- All instances of
clip-pathandstop-colorhave been updated toclipPathandstopColorrespectively.- The functionality and appearance of the SVG remain unchanged.
- These changes contribute to a more robust and warning-free codebase.
Great job on implementing these necessary updates!
blockchain/better-calls/sky.ts (1)
Line range hint
118-133: LGTM! Verify impact on dependent code.The simplification of
skyUsdsWalletStakeDetailsby removingrewardRateandtotalUSDSLockedaligns with the PR objectives of enhancing wallet handling. This change potentially improves performance and simplifies the data model for non-owner views.Please ensure that any code depending on the removed
rewardRateandtotalUSDSLockedvalues has been updated accordingly. Run the following script to check for any remaining usage:Also, consider updating any relevant documentation or comments that might reference these removed properties.
Verification successful
Verification Successful: No usages found for
rewardRateandtotalUSDSLocked.The removal of these properties from
skyUsdsWalletStakeDetailsdoes not affect other parts of the codebase and aligns with the PR objectives.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of removed properties from skyUsdsWalletStakeDetails # Test: Search for usage of rewardRate and totalUSDSLocked from skyUsdsWalletStakeDetails rg --type typescript "skyUsdsWalletStakeDetails.*rewardRate" rg --type typescript "skyUsdsWalletStakeDetails.*totalUSDSLocked"Length of output: 200
Script:
#!/bin/bash # Description: Check for usage of removed properties from skyUsdsWalletStakeDetails # Test: Search for usage of rewardRate and totalUSDSLocked from skyUsdsWalletStakeDetails rg --glob '*.ts' 'skyUsdsWalletStakeDetails.*rewardRate' rg --glob '*.ts' 'skyUsdsWalletStakeDetails.*totalUSDSLocked' rg --glob '*.tsx' 'skyUsdsWalletStakeDetails.*rewardRate' rg --glob '*.tsx' 'skyUsdsWalletStakeDetails.*totalUSDSLocked'Length of output: 247
features/sky/components/SwapCard.tsx (2)
Line range hint
278-290: Approve the usage ofshowAllowanceInfo.The
showAllowanceInfoconstant is correctly used to conditionally render aMessageCardcomponent. This implementation ensures that users are informed about the need to update their allowance only when necessary, improving the user experience.
Line range hint
1-390: Alignment with PR objectives and request for clarification.The changes in this file align well with the PR objective of enhancing wallet handling, particularly in improving the user experience for allowance information display. The introduction of
showAllowanceInfoand its usage in conditional rendering contribute to this goal.However, I couldn't find specific changes addressing the user experience for non-owner views, as mentioned in the PR objectives. Could you please clarify if these changes are implemented in other files or if they're planned for future commits?
To ensure we haven't missed any non-owner view related changes, let's search for relevant keywords:
Consider documenting the different view types (owner vs non-owner) and their specific behaviors in the component or in a separate documentation file. This would help maintain clarity as the component evolves.
helpers/context/ProductContext.ts (2)
3-3: LGTM: Import statement updated correctlyThe import statement has been properly updated to include
skyUsdsStakeDetails, which is consistent with the new functionality added in this file.
737-737: LGTM: New observable correctly exposed in the product contextThe
skyUsdsStakeDetails$observable has been properly added to the object returned bysetupProductContext, making it available for use in other parts of the application. This is consistent with how other observables are exposed in this context.pages/[networkOrProduct]/sky/upgrade.tsx (1)
99-106: LGTM: Swap cards rendered correctlyThe swap cards are being mapped and rendered as expected using the
skySwapTokensConfig. The implementation is clear and efficient.pages/earn/srr/[wallet].tsx (6)
25-25: Handle OptionalwalletAddressProperlyThe
walletAddressprop has been made optional in theSkyStakeUsdsViewcomponent. Please ensure that all usages ofwalletAddresswithin this component and its child components correctly handle cases whenwalletAddressisundefined.
37-37: Confirm Ownership Logic with OptionalwalletAddressThe
isOwnervariable compares the connected wallet address to the optionalwalletAddress. SincewalletAddresscan beundefined, ensure that this comparison accurately reflects ownership status and does not cause any unexpected behavior.
44-49: Check Observable Dependencies inuseObservableHookThe
useObservablehook forskyStakeWalletDatadepends onreloadingTokenInfoandwalletAddress. Make sure that changes to these dependencies correctly trigger updates and that the observables handleundefinedwalletAddressgracefully.
57-63: Ensure State Synchronization inuseEffectHookThe
useEffecthook updatestempSkyStakeWalletDatawhenskyStakeWalletDatachanges. Verify that all conditions are correctly set to prevent missed updates or infinite loops.
126-129: Validate Conditional Rendering LogicThe conditional rendering of the
VaultOwnershipBannercomponent checks for!isOwner && walletAddress. SincewalletAddressis optional, confirm that this condition behaves as expected whenwalletAddressisundefined, and that it doesn't unintentionally hide or show the banner.
163-170: Consistent Handling of OptionalwalletAddressin Wrapper ComponentIn
SkyStakeUsdsViewWrapper, thewalletAddressprop is also made optional. Ensure that all nested components, includingWithTermsOfServiceandWithWalletAssociatedRisk, correctly handle the optionalwalletAddressto prevent any runtime errors.features/sky/hooks/useSky.ts (10)
15-16: Verify handling of optional 'allowance' and 'balance' propertiesBy making
allowanceandbalanceoptional inResolvedDepositParamsType, ensure that all usages of these properties correctly handle cases where they might beundefinedto prevent potential runtime errors.
22-25: Ensure safe access to optional balance and allowance propertiesThe properties
primaryTokenBalance,primaryTokenAllowance,secondaryTokenBalance, andsecondaryTokenAllowanceare now optional inuseSkyType. Verify that all code accessing these properties checks forundefinedto avoid unintended behavior.
28-28: Addition of 'viewWalletAddress' propertyThe
viewWalletAddressproperty has been added touseSkyType. This allows for ownership checks within the hook. Ensure that this property is correctly passed and utilized where necessary.
46-46: Include 'viewWalletAddress' in 'useSky' parametersThe
viewWalletAddressparameter has been added to theuseSkyhook function signature. Confirm that this parameter is properly provided when the hook is used elsewhere in the codebase.
48-48: Destructure 'replace' from 'useRouter'The
replacefunction fromuseRouteris now being used. Ensure that this import is necessary and thatreplaceis used appropriately for navigation purposes within the hook.
219-223: Confirm navigation logic for non-ownersWhen the user is not the owner (
!isOwner), the action function redirects to/earn/srr/${walletAddress}. Verify thatwalletAddressis the intended address for redirection, and that this behavior aligns with the desired user experience.
237-238: Dependencies correctly include 'isOwner' and 'replace'The dependencies array for the
useMemohook includesisOwnerandreplace, which are used within theactionfunction. This ensures that the memoization behaves correctly when these values change.
251-253: Update 'actionLabel' for non-owner usersThe
actionLabelis set to'Go to your position'when the user is not the owner. Ensure that this label is clear and appropriately guides the user to their correct position or page.
255-256: Handle optional 'allowance' in conditionsThe condition checks if
resolvedPrimaryTokenData.allowanceis falsy or insufficient. Sinceallowanceis now optional, ensure that the condition properly handles cases whenallowanceisundefinedto prevent logical errors.
263-271: Include 'isOwner' in 'actionLabel' dependenciesThe
actionLabelusesisOwner, and it's correctly included in the dependencies array of theuseMemohook. This ensures that the label updates appropriately when the ownership status changes.features/sky/components/stake/SkyStakePositionView.tsx (13)
25-25: Import of 'zero' is appropriateThe
zeroconstant is used for default values when wallet data may be undefined.
34-40: Optional properties are appropriately addedThe properties
usdsAllowance,skyAllowance, andskyStakeWalletDatahave been made optional inSkyStakeViewType, which aligns with the updated handling of wallet data. Their usage in the component correctly utilizes optional chaining to handle potentialundefinedvalues.
46-46: Addition of optional 'viewWalletAddress' propertyThe optional
viewWalletAddressproperty has been added toSkyStakeViewType, allowing the component to handle different wallet addresses appropriately.
56-58: Props updated to include optional wallet dataThe component now accepts
skyStakeWalletDataandviewWalletAddressas props, aligning with the updatedSkyStakeViewTypeinterface and enhancing flexibility in handling wallet-specific data.
97-103: Verify handling of 'skyStakeWalletData' in 'primaryTokenBalance'In the
useSkyhook,primaryTokenBalanceis set based onstakingAction. WhenstakingActionis not 'stake', it usesskyStakeWalletData?.balance. Please ensure thatskyStakeWalletDatais defined in these cases to prevent possibleundefinedvalues causing issues within theuseSkyhook.
141-147: Logic for 'showAllowanceInfo' appears correctThe
showAllowanceInfovariable is computed appropriately to determine if the user needs to update the allowance based on the entered amount and current allowance.
154-171: Action pills updated correctly with appropriate disabled statesThe
ActionPillsitems for 'stake', 'unstake', and 'claim' have been updated to include appropriatedisabledconditions based on loading status and ownership. This ensures that actions are only available when the user is permitted to perform them.
181-181: Displaying earned SKY amount correctlyThe earned SKY amount is displayed using
skyStakeWalletData?.earned || zero, which appropriately handles cases where wallet data may be undefined.
194-196: Verify availability of 'skyStakeWalletData' when setting 'maxAmount'When
stakingActionis not 'stake',maxAmountis set toskyStakeWalletData?.balance. Please ensure thatskyStakeWalletDatais defined in these cases to preventmaxAmountfrom beingundefined, which could affect theVaultActionInputcomponent's functionality.
264-264: Conditional rendering of allowance info messageThe
MessageCardfor updating allowance is correctly conditionally rendered based on!isLoading && showAllowanceInfo.
282-282: Primary button configured appropriately based on 'stakingAction'The
primaryButtonlabel and action are set correctly depending on whether the action is 'claim' or not. Loading and disabled states correctly consider bothisLoadingandreloadingTokenInfo.
307-307: Displaying USDS Deposited using optional wallet dataThe
USDS Depositedvalue correctly usesskyStakeWalletData?.balance || zeroto handle cases where wallet data may be undefined.
317-317: Displaying SKY Earned using optional wallet dataThe
SKY Earnedvalue correctly usesskyStakeWalletData?.earned || zeroto handle cases where wallet data may be undefined.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
handlers/portfolio/positions/handlers/sky/index.ts (1)
13-14: LGTM: New stake details fetched correctlyThe addition of
skyUsdsStakeDetails()to the Promise.all call is correct and consistent with the new import. The destructuring has been properly updated to match.Consider renaming
usdsStakeDetailstousdsGeneralStakeDetailsor similar to clearly differentiate it from the wallet-specific details. This would improve code readability:- ]).then(([usdsWalletStakeDetails, usdsStakeDetails]) => { + ]).then(([usdsWalletStakeDetails, usdsGeneralStakeDetails]) => {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- features/sky/components/SwapCard.tsx (3 hunks)
- features/sky/components/stake/SkyStakePositionView.tsx (11 hunks)
- features/sky/helpers.ts (1 hunks)
- features/sky/hooks/useSky.ts (7 hunks)
- handlers/portfolio/positions/handlers/sky/index.ts (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- features/sky/components/SwapCard.tsx
- features/sky/components/stake/SkyStakePositionView.tsx
- features/sky/hooks/useSky.ts
Additional comments not posted (5)
features/sky/helpers.ts (2)
1-1: LGTM: Import statement is correct.The import statement for BigNumber is correctly using the
typekeyword, which is a good practice for TypeScript as it ensures the import is used only for type checking and not at runtime.
1-4: Overall assessment: Function serves its purpose but could be improved.The
showAllowanceInfofunction correctly implements the logic to determine when allowance information should be shown. However, there's room for improvement in terms of naming, type annotations, and error handling. Consider implementing the suggested refactoring to enhance the function's robustness and readability.handlers/portfolio/positions/handlers/sky/index.ts (3)
1-1: LGTM: New import added correctlyThe addition of
skyUsdsStakeDetailsimport is appropriate as it's used in the function below. The import statement follows the existing code style.
Line range hint
1-52: Summary: Enhancements to skyPositionsHandler look goodThe changes to
skyPositionsHandlerimprove its functionality by incorporating both wallet-specific and general stake details. The modifications are well-structured, consistent, and maintain the existing code style.Key points:
- New import and usage of
skyUsdsStakeDetailsfunction.- Updated Promise.all call to fetch both types of stake details.
- Change in APY calculation source from wallet-specific to general stake details.
Consider the suggested variable renaming for clarity and ensure the APY calculation change aligns with the intended business logic.
Overall, these changes enhance the handler's capabilities and should provide more comprehensive stake information.
34-34: Verify business logic: APY source changedThe change from
usdsWalletStakeDetails.rewardRatetousdsStakeDetails.rewardRatefor the APY calculation is consistent with the new data fetching logic. This implies that the APY is now based on general stake information rather than wallet-specific data.Please confirm that this change aligns with the intended business logic. Does the APY need to be the same for all users (general stake details) rather than potentially different for each wallet (wallet-specific details)?
To verify the impact of this change, you can run the following script:
This will help ensure that the change is consistent across the codebase and that no other APY calculations are affected unintentionally.
Verification successful
Update Shell Script to Correctly Search TypeScript Files
The previous script failed to recognize the
typescriptfile type. Let's adjust thergcommands to target.tsand.tsxfiles specifically.
To further investigate the impact and consistency of this change, we need to perform additional searches. Let's run the following scripts:
These scripts will help us understand if the change is isolated to this specific instance or if there are other places where similar changes might be needed.
The investigation confirms that the change from
usdsWalletStakeDetails.rewardRatetousdsStakeDetails.rewardRateis appropriate and isolated to this specific instance in the sky handler file. The change is consistent with using more general stake details for the reward rate calculation, while wallet-specific details are still used for other properties like balance and earned amounts.This modification appears to be intentional and does not negatively impact other parts of the codebase. The change aligns with the goal of using general stake information rather than wallet-specific data for the APY calculation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other usages of rewardRate from usdsWalletStakeDetails # Test: Search for usages of rewardRate from usdsWalletStakeDetails rg --type typescript "usdsWalletStakeDetails\.rewardRate" # Test: Search for other potential APY calculations rg --type typescript "(apy|APY|reward.?rate)"Length of output: 177
Script:
#!/bin/bash # Description: Check for other usages of rewardRate from usdsWalletStakeDetails # Test: Search for usages of rewardRate from usdsWalletStakeDetails in .ts and .tsx files rg "usdsWalletStakeDetails\.rewardRate" -g "*.ts" -g "*.tsx" # Test: Search for other potential APY calculations in .ts and .tsx files rg "(apy|APY|reward.?rate)" -g "*.ts" -g "*.tsx"Length of output: 114636
Script:
#!/bin/bash # Search for other occurrences of usdsWalletStakeDetails.rewardRate echo "Searching for usdsWalletStakeDetails.rewardRate:" rg "usdsWalletStakeDetails\.rewardRate" -g "*.ts" -g "*.tsx" # Look for similar patterns in other portfolio handler files echo "\nSearching for rewardRate in portfolio handlers:" rg "rewardRate" -g "handlers/portfolio/positions/handlers/*.ts" # Examine the sky handler file more closely echo "\nExamining the sky handler file:" rg "usdsWalletStakeDetails" handlers/portfolio/positions/handlers/sky/index.ts rg "usdsStakeDetails" handlers/portfolio/positions/handlers/sky/index.tsLength of output: 1238
Summary by CodeRabbit
Release Notes
New Features
skyUsds.Improvements
Bug Fixes
Style