refactor: updated apr formula #40
Conversation
WalkthroughThe update introduces significant changes to the handling of APR calculations by integrating new endpoints and restructuring service dependencies. Key components like Changes
Sequence Diagram(s)sequenceDiagram
participant StakingService
participant Okp4Service
participant TokenService
participant CosmosNetwork
StakingService->>Okp4Service: getApr()
Okp4Service->>CosmosNetwork: fetch(INFLATION)
CosmosNetwork-->>Okp4Service: INFLATION Response
Okp4Service->>CosmosNetwork: fetch(DISTRIBUTION_PARAMS)
CosmosNetwork-->>Okp4Service: DISTRIBUTION_PARAMS Response
Okp4Service-->>StakingService: Calculated APR
TokenService->>Okp4Service: getApr()
Okp4Service->>CosmosNetwork: fetch(INFLATION)
CosmosNetwork-->>Okp4Service: INFLATION Response
Okp4Service->>CosmosNetwork: fetch(DISTRIBUTION_PARAMS)
CosmosNetwork-->>Okp4Service: DISTRIBUTION_PARAMS Response
Okp4Service-->>TokenService: Calculated APR
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 1
Outside diff range and nitpick comments (1)
src/core/lib/okp4/okp4.service.ts (1)
30-30: Consider using a more precise import forBig.js.Importing the entire
Big.jslibrary globally might lead to larger bundle sizes if this service is used on the client side. Consider importing only the necessary functions ifBig.jssupports tree-shaking.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/core/lib/okp4/enums/endpoints.enum.ts (1 hunks)
- src/core/lib/okp4/okp4.service.ts (2 hunks)
- src/core/lib/okp4/responses/distribution-params.response.ts (1 hunks)
- src/core/lib/okp4/responses/inflation.response.ts (1 hunks)
- src/modules/staking/services/staking.service.ts (3 hunks)
- src/modules/staking/staking.module.ts (2 hunks)
- src/modules/token/services/token.service.ts (2 hunks)
- src/modules/token/token.module.ts (1 hunks)
Files skipped from review due to trivial changes (2)
- src/core/lib/okp4/enums/endpoints.enum.ts
- src/core/lib/okp4/responses/distribution-params.response.ts
Additional context used
Biome
src/core/lib/okp4/okp4.service.ts
[error] 175-176: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 266-266: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
src/modules/staking/services/staking.service.ts
[error] 477-477: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Additional comments not posted (7)
src/core/lib/okp4/responses/inflation.response.ts (1)
1-3: Interface definition is correct and aligns with the requirements.The
InflationResponseinterface correctly defines theinflationproperty as a string, which is consistent with the expected data type for inflation values.src/modules/staking/staking.module.ts (1)
22-22: Simplified controller array is appropriate.The update to the
controllersarray to include onlyStakingControllersimplifies and clarifies the module's structure.src/modules/token/token.module.ts (1)
10-10: Correct addition ofOkp4Serviceto imports and providers.The inclusion of
Okp4Servicein both imports and providers is essential for enabling the new APR fetching functionality in theTokenModule.Also applies to: 18-18
src/modules/token/services/token.service.ts (2)
19-19: Appropriate addition ofOkp4Serviceas a dependency.The inclusion of
Okp4Serviceas a dependency is crucial for enabling APR fetching functionality in theTokenService.Also applies to: 27-27
100-100: Updated APR fetching logic is correct.The update to use
okp4Service.getApr()for fetching APR infetchAndCacheTokenInfocorrectly reflects the new dependency and functionality.src/core/lib/okp4/okp4.service.ts (1)
28-30: New imports for handling APR calculations.The new imports
InflationResponseandDistributionParamsResponseare correctly added to handle the responses from the new endpoints. This aligns with the PR's objectives to integrate new endpoints for APR calculations.src/modules/staking/services/staking.service.ts (1)
156-156: Updated method call to useokp4Service.getApr().The change to use
okp4Service.getApr()instead ofosmosisService.getStakingApr()is aligned with the PR's objectives to centralize APR calculations within theOkp4Service. This should ensure that APR values are consistent across different modules.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/core/lib/okp4/okp4.service.ts (2 hunks)
Additional context used
Biome
src/core/lib/okp4/okp4.service.ts
[error] 175-176: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Additional comments not posted (1)
src/core/lib/okp4/okp4.service.ts (1)
28-30: Imports for new classes and libraries added.The addition of
InflationResponse,DistributionParamsResponse, and theBiglibrary from "big.js" align with the new functionality introduced in thegetApr()method. This ensures proper data handling and calculations.
| async getApr() { | ||
| const promises = [ | ||
| this.getWithErrorHandling(this.constructUrl(Endpoints.INFLATION)), | ||
| this.getWithErrorHandling( | ||
| this.constructUrl(Endpoints.DISTRIBUTION_PARAMS) | ||
| ), | ||
| this.getStakingPool(), | ||
| ]; | ||
| const res = (await Promise.all(promises)) as [ | ||
| InflationResponse, | ||
| DistributionParamsResponse, | ||
| StakingPoolResponse | ||
| ]; | ||
|
|
||
| if (res[2]?.pool?.bonded_tokens) { | ||
| Big(res[0].inflation) | ||
| .mul(Big(1).minus(res[1].params.community_tax)) | ||
| .div(res[2].pool.bonded_tokens) | ||
| .toString(); | ||
| } | ||
| return "0"; | ||
| } |
There was a problem hiding this comment.
Review of the getApr() method.
The method correctly uses the new endpoints to fetch inflation and distribution parameters. However, there's a potential improvement in how the results are handled:
- The method always returns "0" as a string, which might be incorrect if the calculations in lines 266-270 are supposed to affect the return value. This could be a logic error.
- Use optional chaining as suggested by static analysis to safely access nested properties.
- if (res[2] && res[2].pool.bonded_tokens) {
+ if (res[2]?.pool?.bonded_tokens) {
Big(res[0].inflation)
.mul(Big(1).minus(res[1].params.community_tax))
.div(res[2].pool.bonded_tokens)
.toString();
}
return "0";Committable suggestion was skipped due to low confidence.
No description provided.