Update APY calculations for SKY #4030
Conversation
WalkthroughThe changes primarily enhance the Changes
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 (
|
![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 (2)
features/sky/components/stake/SkyStakePositionView.tsx (1)
306-306: LGTM! Consider adjusting percent formatting.The change from
formatCryptoBalance(skyStakeData.rewardRate)toformatDecimalAsPercent(skyStakeData.apy, { noPercentSign: true })is consistent with the type definition update and more appropriate for displaying APY.Consider removing the
noPercentSign: trueoption fromformatDecimalAsPercentand updating theunitprop:-value={`${formatDecimalAsPercent(skyStakeData.apy, { noPercentSign: true })}`} -unit="%" +value={formatDecimalAsPercent(skyStakeData.apy)} +unit=""This change would let
formatDecimalAsPercenthandle the percent sign, ensuring consistent formatting across the application.helpers/context/ProductContext.ts (1)
692-693: LGTM! Consider adding a comment for clarity.The change to
skyUsdsStakeDetails$function looks good. It now accepts an optionalmkrPriceparameter, which allows for more flexibility in APY calculations for SKY.Consider adding a brief comment explaining the purpose of the
mkrPriceparameter and its impact on the stake details calculation. This would improve code readability and maintainability. For example:// mkrPrice: Optional parameter to provide current MKR price for more accurate APY calculations const skyUsdsStakeDetails$ = (mkrPrice?: BigNumber) => onEveryBlock$.pipe(() => fromPromise(skyUsdsStakeDetails({ mkrPrice })))
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- blockchain/better-calls/sky.ts (3 hunks)
- features/sky/components/stake/SkyStakePositionView.tsx (3 hunks)
- handlers/portfolio/positions/handlers/sky/index.ts (2 hunks)
- handlers/product-hub/update-handlers/sky/skyHandler.ts (3 hunks)
- helpers/context/ProductContext.ts (1 hunks)
- pages/earn/srr/[wallet].tsx (4 hunks)
Additional comments not posted (11)
handlers/portfolio/positions/handlers/sky/index.ts (4)
9-9: LGTM: Function signature updated correctly.The
skyPositionsHandlerfunction signature has been updated to include thepricesparameter, which is used later in the function to access price data.To ensure all calls to this function have been updated, please run the following script:
#!/bin/bash # Description: Verify all calls to skyPositionsHandler include the prices parameter # Test: Search for calls to skyPositionsHandler rg --type typescript -A 5 'skyPositionsHandler\('
14-14: LGTM: APY calculation and formatting updated.The changes in the function body look good:
mkrPriceis now passed as aBigNumberinstance usingprices.MKR.- APY formatting now uses
formatDecimalAsPercentinstead offormatPercent.These updates should provide more accurate APY calculations and formatting.
To ensure these changes produce the expected results, please:
- Add unit tests for the
skyPositionsHandlerfunction, focusing on the APY calculation and formatting.- Perform integration tests to verify the correct display of APY values in the UI.
Run the following script to check for existing tests:
#!/bin/bash # Description: Check for existing tests for skyPositionsHandler # Test: Search for test files related to skyPositionsHandler fd -e test.ts -e spec.ts | xargs rg 'describe.*skyPositionsHandler'If no tests are found, consider adding them to ensure the correctness of the APY calculations and formatting.
Also applies to: 35-35
Line range hint
1-52: Summary: APY calculations for SKY have been successfully updated.The changes in this file align well with the PR objectives. The
skyPositionsHandlerfunction has been updated to:
- Accept a
pricesparameter for accessing price data.- Use
BigNumberformkrPricecalculation.- Implement a new APY formatting method.
These modifications should improve the accuracy of APY metrics for the SKY asset across various components of the application.
To ensure the changes are fully integrated and functioning as expected:
- Verify that all components using
skyPositionsHandler(portfolio view, product hub, and position page) are updated to pass the requiredpricesparameter.- Conduct end-to-end tests to confirm the correct display of updated APY values in the UI.
- Monitor the production environment after deployment to ensure there are no unexpected issues with APY calculations or display.
Run the following script to identify components that might need updates:
#!/bin/bash # Description: Find components that might need updates due to skyPositionsHandler changes # Test: Search for files importing skyPositionsHandler rg --type typescript 'import.*skyPositionsHandler'Review the identified files to ensure they're correctly using the updated
skyPositionsHandler.
6-6: LGTM: Import statement updated correctly.The import statement has been updated to use
formatDecimalAsPercentinstead offormatPercent, which aligns with the changes in APY formatting later in the code.To ensure the new formatter function is correctly implemented, please run the following script:
Verification successful
LGTM: Verified implementation of
formatDecimalAsPercent.The function
formatDecimalAsPercentis correctly implemented inhelpers/formatters/format.ts, and the import statement inhandlers/portfolio/positions/handlers/sky/index.tsaccurately references this function.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of formatDecimalAsPercent # Test: Search for the implementation of formatDecimalAsPercent rg --type typescript -A 10 'export function formatDecimalAsPercent'Length of output: 181
Script:
#!/bin/bash # Description: Verify the implementation of formatDecimalAsPercent # Test: Search for the implementation of formatDecimalAsPercent with corrected file type rg --type ts -A 10 'export function formatDecimalAsPercent'Length of output: 642
handlers/product-hub/update-handlers/sky/skyHandler.ts (4)
4-4: LGTM: Import statement for Tickers typeThe addition of the import statement for the
Tickerstype is appropriate and consistent with the changes made to the function signatures in this file.
12-15: LGTM: Updated getSkyData function signatureThe addition of the
tickersparameter to thegetSkyDatafunction signature is appropriate and consistent with the changes made within the function body. The return type remains unchanged, which maintains consistency with the existing codebase.
46-47: LGTM: Updated default export functionThe changes to the default export function are appropriate and consistent with the updates made to the
getSkyDatafunction. The addition of thetickersparameter and its propagation to thegetSkyDatafunction call ensures that the necessary data is available throughout the execution flow.
Line range hint
17-34: Verify the impact of replacing 'fee' with 'weeklyNetApy'The changes to the
getSkyDatafunction look good overall. The use oftickersto get themkrPriceis consistent with the function signature change, and the fallback option formkrPriceis a good practice.However, the replacement of
feewithweeklyNetApyin the return structure might affect other parts of the codebase that expect thefeeproperty. Please ensure that all consumers of this data are updated accordingly.To verify the impact of this change, you can run the following script:
This will help identify any potential areas that need to be updated due to this change.
Verification successful
Verified: No external usages of 'fee' found
The replacement of
feewithweeklyNetApyin thegetSkyDatafunction has been successfully verified. No external usages of thefeeproperty fromskyHandler.tswere found in the codebase, ensuring that this change does not impact other parts of the application.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of 'fee' property from skyHandler rg --type typescript -g '!**/skyHandler.ts' 'fee.*skyHandler'Length of output: 98
Script:
#!/bin/bash # Description: Search for usages of 'fee' property from skyHandler rg --type ts -g '!**/skyHandler.ts' 'fee.*skyHandler'Length of output: 55
features/sky/components/stake/SkyStakePositionView.tsx (1)
Line range hint
1-342: Overall assessment: Changes look good and align with PR objectives.The modifications to update APY calculations for SKY have been implemented correctly. The changes are focused and maintain good code quality. Here's a summary of the key points:
- The
skyStakeDatatype has been updated to useapyinstead ofrewardRate.- The display of the APY has been updated to use
formatDecimalAsPercent.These changes improve the accuracy and clarity of the APY representation in the Sky Stake Position View.
pages/earn/srr/[wallet].tsx (2)
33-35: Updated state property to 'apy'The
tempSkyStakeDatastate now includes anapyproperty. This aligns with the shift from usingrewardRatetoapy, as per the PR objectives.
68-70: State update condition correctly reflects 'apy'The
useEffectcondition now comparesskyStakeData.apywithtempSkyStakeData?.apy, which is consistent with the change fromrewardRatetoapy.
Added proper APY calc for SKY (portfolio, product hub, position page)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
These changes collectively improve the user experience by providing clearer and more relevant staking information.