fix: add token claims per network #4046
Conversation
WalkthroughThe changes in this pull request primarily focus on the Changes
Possibly related PRs
Suggested reviewers
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- features/omni-kit/components/details-section/OmniDetailSectionRewardsClaims.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
features/omni-kit/components/details-section/OmniDetailSectionRewardsClaims.tsx (2)
18-18: Necessary Import ofNetworkIdsThe addition of
import { NetworkIds } from 'blockchain/networks'is appropriate and required for definingclaimableErc20.
20-33: Correct Implementation of Network-SpecificclaimableErc20MappingDefining
claimableErc20as aRecord<NetworkIds, string[]>allows for network-specific claimable tokens, enhancing the flexibility and scalability of the code.
| @@ -36,7 +50,7 @@ | |||
| useEffect(() => { | |||
| if (dpmProxy) { | |||
| // Existing ERC20 claims logic | |||
| claimableErc20.forEach((token) => { | |||
| claimableErc20[networkId].forEach((token) => { | |||
There was a problem hiding this comment.
Handle Undefined claimableErc20[networkId] to Prevent Runtime Errors
If networkId is not a key in the claimableErc20 object, accessing claimableErc20[networkId] will return undefined, leading to a runtime error when calling forEach.
Apply this diff to ensure safe access and prevent potential runtime errors:
- claimableErc20[networkId].forEach((token) => {
+ (claimableErc20[networkId] || []).forEach((token) => {This modification defaults to an empty array when claimableErc20[networkId] is undefined, ensuring that the forEach method always functions correctly, even for unrecognized networks.
📝 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.
| claimableErc20[networkId].forEach((token) => { | |
| (claimableErc20[networkId] || []).forEach((token) => { |
There was a problem hiding this comment.
ok I'll agree. But don't get cocky. Thats only because I like you. If somone modifies the NetwrokIds and there's a cese we use it as index - that should be caught by ts
There was a problem hiding this comment.
@halaprix, glad to hear that you've agreed to the change! You're absolutely right, TypeScript should catch such cases if someone modifies the NetworkIds. Thank you!
🐰😊
✏️ Learnings added
Learnt from: halaprix
PR: OasisDEX/oasis-borrow#4046
File: features/omni-kit/components/details-section/OmniDetailSectionRewardsClaims.tsx:53-53
Timestamp: 2024-10-17T18:29:27.516Z
Learning: In TypeScript code, we rely on TypeScript's type checking to catch cases where `NetworkIds` are modified and used as an index, ensuring that accessing `claimableErc20[networkId]` is safe.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
features/omni-kit/components/details-section/OmniDetailSectionRewardsClaims.tsx (3)
7-7: LGTM! Consider using a constant for the MAINNET tokens.The changes to
claimableErc20align well with the PR objectives, providing a clear structure for network-specific claimable tokens. This implementation allows for easy management of claimable tokens across different networks.Consider defining a constant for the MAINNET tokens to improve maintainability:
const MAINNET_CLAIMABLE_TOKENS = ['ENA', 'SENA'] as const; const claimableErc20: Record<NetworkIds, readonly string[]> = { [NetworkIds.MAINNET]: MAINNET_CLAIMABLE_TOKENS, // ... other networks };This approach would make it easier to update the list of claimable tokens for MAINNET in the future if needed.
Also applies to: 20-33
Line range hint
74-136: LGTM! Consider extracting protocol-specific logic.The new Aave and Spark rewards check logic is well-implemented and aligns with the PR objectives. It correctly handles different protocols and networks, with proper error handling and logging.
To improve code organization and maintainability, consider extracting the protocol-specific logic into separate functions:
function getAaveV3Addresses(network: Network): { rewardsController: string, poolDataProvider: string } { if (network !== Network.HARDHAT && network !== Network.LOCAL && network !== Network.TENDERLY) { return { rewardsController: ADDRESSES[network].aave.v3.RewardsController, poolDataProvider: ADDRESSES[network].aave.v3.PoolDataProvider }; } throw new Error(`Unsupported network for AaveV3: ${network}`); } function getSparkV3Addresses(network: Network): { rewardsController: string, poolDataProvider: string } { if (network !== Network.HARDHAT && network !== Network.LOCAL && network !== Network.TENDERLY) { return { rewardsController: ADDRESSES[network].spark.RewardsController, poolDataProvider: ADDRESSES[network].spark.PoolDataProvider }; } throw new Error(`Unsupported network for SparkV3: ${network}`); } // In the useEffect hook: let { rewardsControllerAddress, poolDataProviderAddress } = protocol === LendingProtocol.AaveV3 ? getAaveV3Addresses(network) : protocol === LendingProtocol.SparkV3 ? getSparkV3Addresses(network) : (() => { throw new Error(`Unsupported protocol: ${protocol}`) })();This refactoring would make the code more modular and easier to maintain, especially if more protocols are added in the future.
Line range hint
168-180: LGTM! Consider using a configuration object for eligibility.The updates to the
OmniDetailSectionRewardsClaimscomponent correctly expand the eligibility for rewards claiming to include more protocols and tokens, aligning with the PR objectives.To improve flexibility and maintainability, consider using a configuration object for eligibility:
const REWARDS_ELIGIBILITY = { protocols: [ LendingProtocol.MorphoBlue, LendingProtocol.Ajna, LendingProtocol.AaveV3, LendingProtocol.SparkV3, ], tokens: ['SUSDE', 'USDE', 'WETH', 'ETH'], }; const isEligible = REWARDS_ELIGIBILITY.protocols.includes(protocol) && (REWARDS_ELIGIBILITY.tokens.includes(collateralToken) || REWARDS_ELIGIBILITY.tokens.includes(quoteToken));This approach would make it easier to update the list of eligible protocols and tokens in the future without modifying the component logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- features/omni-kit/components/details-section/OmniDetailSectionRewardsClaims.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
features/omni-kit/components/details-section/OmniDetailSectionRewardsClaims.tsx (2)
53-53: Improved network-specific token claiming, but consider error handling.The updated logic correctly uses the network-specific array of claimable tokens, which aligns with the PR objectives. This change ensures that only the appropriate tokens are considered for claiming on each network.
As mentioned in a previous review comment, consider handling the case where
claimableErc20[networkId]might be undefined to prevent potential runtime errors. The suggestion to use(claimableErc20[networkId] || []).forEach(...)is still valid and would improve the robustness of this code.
Line range hint
1-180: Overall, excellent implementation with room for minor improvements.The changes in this file successfully implement network-specific token claiming and expand the rewards claiming functionality as per the PR objectives. The code is well-structured and includes proper error handling. The suggestions provided in the review comments, if implemented, would further enhance the code's maintainability and robustness.
Great job on this implementation!
Update claimable ERC20 tokens by network
Changes 👷♀️
claimableErc20array with a network-specific objectclaimableErc20How to test 🧪
@pr (Diff with Main Branch)
This update introduces network-specific claimable ERC20 tokens, allowing for more granular control over which tokens can be claimed on different networks. The main changes are the introduction of the
claimableErc20object and the updated logic to useclaimableErc20[networkId]instead of the previous static array.Summary by CodeRabbit
New Features
Bug Fixes