Feature: InjectiveStaking add new compound_rewards function #4
Conversation
WalkthroughThe changes involve significant updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InjectiveStaking
participant Validator
User->>InjectiveStaking: call compound_rewards(validator_address)
InjectiveStaking->>InjectiveStaking: validate validator address
InjectiveStaking->>Validator: fetch initial balance
Validator-->>InjectiveStaking: return initial balance
InjectiveStaking->>Validator: withdraw rewards
Validator-->>InjectiveStaking: confirm withdrawal
InjectiveStaking->>InjectiveStaking: wait_for_balance_update()
InjectiveStaking->>InjectiveStaking: calculate rewards to stake
InjectiveStaking->>Validator: restake rewards
Validator-->>InjectiveStaking: confirm restaking
InjectiveStaking-->>User: return success message
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 4
🧹 Outside diff range and nitpick comments (4)
injective_functions/staking/staking_schema.json (1)
23-30: Consider enhancing the schema with additional validation and documentation.The schema could benefit from:
- Adding a pattern constraint for
validator_addressto validate the format- Including additional documentation about:
- Minimum rewards threshold required for compounding
- Gas fee considerations
- Expected behavior when rewards are insufficient
Consider applying this enhancement:
"validator_address": { "type": "string", - "description": "Validator address you want to compound your rewards with." + "description": "Validator address you want to compound your rewards with. Ensure sufficient rewards are available to cover gas fees.", + "pattern": "^injvaloper[a-zA-Z0-9]{39}$", + "examples": ["injvaloper1..."] }injective_functions/function_schemas.json (2)
111-114: Add validator address format validationWhile the parameter structure is correct, consider adding a pattern validation for the validator address to ensure it matches Injective's address format.
"validator_address": { "type": "string", + "pattern": "^injvaloper[a-zA-Z0-9]{39}$", "description": "Validator address you want to compound your rewards with." }
105-118: Consider adding optional parameters for better controlBased on the PR objectives, the function handles gas fees and balance updates. Consider adding optional parameters to give users more control over the compounding process.
{ "name": "compound_rewards", "description": "Automatically reinvest your staking rewards with a specific validator to increase your staked amount.", "parameters": { "type": "object", "properties": { "validator_address": { "type": "string", "description": "Validator address you want to compound your rewards with." }, + "min_rewards": { + "type": "number", + "description": "Minimum rewards required to proceed with compounding (default: enough to cover gas fees)", + "minimum": 0 + }, + "max_gas": { + "type": "number", + "description": "Maximum gas fee willing to pay for the compound operation", + "minimum": 0 + } }, "required": ["validator_address"] } }injective_functions/functions_schemas.json (1)
784-792: Consider adding optional parameters for better control.To better handle the edge cases mentioned in the PR objectives, consider adding these optional parameters:
"parameters": { "type": "object", "properties": { "validator_address": { "type": "string", "description": "Validator address you want to compound your rewards with." }, + "max_gas_fee": { + "type": "number", + "description": "Maximum gas fee willing to pay for the compound operation." + }, + "min_reward_threshold": { + "type": "number", + "description": "Minimum reward amount required to trigger compounding (to ensure rewards exceed gas costs)." + } }, "required": ["validator_address"] }This would help prevent scenarios where:
- Gas fees exceed the user's tolerance
- Available rewards are insufficient to justify the compounding operation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
injective_functions/function_schemas.json(1 hunks)injective_functions/functions_schemas.json(1 hunks)injective_functions/staking/__init__.py(2 hunks)injective_functions/staking/staking_schema.json(1 hunks)injective_functions/utils/function_helper.py(1 hunks)
🔇 Additional comments (3)
injective_functions/staking/staking_schema.json (1)
21-33: LGTM! The schema structure is well-defined.
The new compound_rewards function schema follows the established pattern and correctly defines the required parameters.
injective_functions/utils/function_helper.py (1)
48-48: LGTM! Verify staking client implementation.
The mapping for the new compound_rewards function is correctly added and follows the established patterns for staking functions.
Let's verify that the corresponding method exists in the staking client:
✅ Verification successful
Implementation verified and properly documented
The compound_rewards method is correctly implemented in the staking client (injective_functions/staking/__init__.py) and follows the expected pattern. The implementation is also well-documented in schema files with proper parameter descriptions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the compound_rewards method is implemented in the staking client
# Expected: Find the method implementation in the staking client files
# Search for the compound_rewards method definition
ast-grep --pattern 'async def compound_rewards($$$)'
# Search for any references to compound_rewards
rg -A 5 'compound_rewards'
Length of output: 2781
injective_functions/functions_schemas.json (1)
781-794: LGTM! The schema structure is well-defined.
The schema follows the established pattern and is properly placed among other staking-related functions.
- Validate interval to prevent division by zero - Catch specific exceptions instead of general Exception - Maintain precision by avoiding conversion to float
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
injective_functions/staking/__init__.py (2)
48-60: Consider adding minimum stake amount check.While the code handles negative and zero rewards well, it should also check if the rewards meet the minimum stake amount required by the network.
# Step 4: Calculate the withdrawn rewards rewards_to_stake = updated_balance - initial_balance + MIN_STAKE_AMOUNT = Decimal("0.001") # Example minimum, adjust as needed + if rewards_to_stake <= 0: if rewards_to_stake < 0: # Specific error for negative rewards return { "success": False, "error": f"Rewards ({rewards_to_stake}) are lower than gas fees, resulting in a negative net " f"reward." } # Generic error for zero rewards return {"success": False, "error": "No rewards available to compound."} + elif rewards_to_stake < MIN_STAKE_AMOUNT: + return { + "success": False, + "error": f"Rewards ({rewards_to_stake}) are below minimum stake amount ({MIN_STAKE_AMOUNT})." + }
96-105: Improve timeout calculation precision.The current implementation might not use the full timeout period due to integer division. Consider using a time-based approach instead of counting iterations.
- for _ in range(timeout // interval): + end_time = asyncio.get_event_loop().time() + timeout + while asyncio.get_event_loop().time() < end_time: balance_response = await self.chain_client.client.get_bank_balance( address=self.chain_client.address.to_acc_bech32(), denom=denom ) updated_balance = Decimal(balance_response.balance.amount) if updated_balance != old_balance: return updated_balance await asyncio.sleep(interval) raise TimeoutError("Balance did not update within the timeout period.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
injective_functions/staking/__init__.py(2 hunks)
🔇 Additional comments (2)
injective_functions/staking/__init__.py (2)
1-5: LGTM! Import changes are appropriate.
The addition of asyncio is necessary for the new async methods, and removing unused imports keeps the code clean.
24-29: LGTM! Well-documented function signature.
The function signature and documentation clearly describe the purpose and parameters.
- Improve precision handling in amount calculation. - Add validator address validation.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
injective_functions/staking/__init__.py (2)
25-29: Enhance method documentation with exception details.The docstring should document the possible exceptions that can be raised (
TimeoutError,ValueError) and the structure of the return dictionary.Apply this diff to improve the documentation:
""" Compounds staking rewards by withdrawing them and restaking. :param validator_address: The validator's address + :raises ValueError: If the validator address format is invalid + :raises TimeoutError: If the balance update times out :return: Transaction result dictionary containing: + - success: bool indicating operation success + - error: str error message if success is False + - withdraw_response: dict containing withdrawal transaction details if success is True + - delegate_response: dict containing delegation transaction details if success is True """
81-108: Consider implementing exponential backoff.The current implementation uses a fixed interval for polling. Consider implementing exponential backoff to reduce network load while still maintaining responsiveness.
Apply this diff to implement exponential backoff:
async def wait_for_balance_update( self, old_balance: Decimal, denom: str, timeout: int = 10, - interval: int = 1 + base_interval: float = 0.5, + max_interval: float = 2.0 ) -> Decimal: """ Waits for the balance to update after a transaction. :param old_balance: Previous balance to compare against :param denom: Denomination of the token (e.g., "inj") :param timeout: Total time to wait (in seconds) - :param interval: Time between balance checks (in seconds) + :param base_interval: Initial time between checks (in seconds) + :param max_interval: Maximum time between checks (in seconds) :return: Updated balance """ - if interval <= 0: - raise ValueError("Interval must be greater than zero.") + if base_interval <= 0: + raise ValueError("Base interval must be greater than zero.") - for _ in range(timeout // interval): + start_time = asyncio.get_event_loop().time() + current_interval = base_interval + + while (asyncio.get_event_loop().time() - start_time) < timeout: balance_response = await self.chain_client.client.get_bank_balance( address=self.chain_client.address.to_acc_bech32(), denom=denom ) updated_balance = Decimal(balance_response.balance.amount) if updated_balance != old_balance: return updated_balance - await asyncio.sleep(interval) + await asyncio.sleep(current_interval) + current_interval = min(current_interval * 2, max_interval) raise TimeoutError("Balance did not update within the timeout period.")
|
All coderabbitai comments resolved. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
injective_functions/staking/__init__.py (3)
25-29: Enhance the docstring with more details.Consider adding more comprehensive documentation including:
- Expected format for validator_address
- Possible error scenarios
- Structure of the return dictionary
""" Compounds staking rewards by withdrawing them and restaking. :param validator_address: The validator's address + :param validator_address: The validator's address (format: injvaloper...) :return: Transaction result + :return: Dict containing: + - success: bool indicating operation success + - error: str with error message if success is False + - withdraw_response: Dict with withdrawal transaction details if success is True + - delegate_response: Dict with delegation transaction details if success is True + :raises: + - ValueError: If validator address format is invalid + - TimeoutError: If balance update times out """
70-74: Standardize the success response structure.The error responses include a "success" and "error" field, while the success response has additional fields. Consider standardizing the response structure.
return { "success": True, + "error": None, + "result": { "withdraw_response": withdraw_response, "delegate_response": delegate_response, + } }
97-105: Improve timeout calculation precision.The current implementation using
timeout // intervalmight not use the full timeout period due to integer division. Consider using a time-based approach instead of counting iterations.- for _ in range(timeout // interval): + end_time = asyncio.get_event_loop().time() + timeout + while asyncio.get_event_loop().time() < end_time: balance_response = await self.chain_client.client.get_bank_balance( address=self.chain_client.address.to_acc_bech32(), denom=denom ) updated_balance = Decimal(balance_response.balance.amount) if updated_balance != old_balance: return updated_balance await asyncio.sleep(interval)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
injective_functions/staking/__init__.py(2 hunks)
🧰 Additional context used
📓 Learnings (1)
injective_functions/staking/__init__.py (2)
Learnt from: defser
PR: InjectiveLabs/iAgent#4
File: injective_functions/staking/__init__.py:41-46
Timestamp: 2024-11-25T17:25:22.648Z
Learning: In `injective_functions/utils/initializers.py`, the method `build_and_broadcast_tx` already performs transaction simulation by calling `simulate`. Therefore, when using `build_and_broadcast_tx`, it's unnecessary to perform an explicit transaction simulation beforehand.
Learnt from: defser
PR: InjectiveLabs/iAgent#4
File: injective_functions/staking/__init__.py:64-70
Timestamp: 2024-11-25T17:25:08.015Z
Learning: In `injective_functions/utils/initializers.py`, the `build_and_broadcast_tx` method already simulates transactions before execution, making additional simulation unnecessary.
🔇 Additional comments (2)
injective_functions/staking/__init__.py (2)
1-5: LGTM! Import changes are appropriate.
The imports correctly support the new async functionality and decimal handling requirements.
31-32: Verify validator existence before proceeding.
While the validator address format is validated, it would be beneficial to verify that the validator exists and is active before attempting operations.
Consider adding a validator verification step using the chain client's query capabilities before proceeding with the operations.
… and put compound_rewards schema only inside staking_schema.json
Comments resolved |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
injective_functions/staking/__init__.py (5)
27-31: Enhance the docstring with more details.The current docstring could be more informative. Consider adding:
- Expected format for validator_address
- Description of the return structure
- Possible error scenarios
""" Compounds staking rewards by withdrawing them and restaking. :param validator_address: The validator's address + :param validator_address: The validator's address (format: injvaloper...) :return: Transaction result + :return: Dict containing: + - success: bool indicating operation success + - error: str with error message if success is False + - withdraw_response: Dict with withdrawal transaction details if success is True + - delegate_response: Dict with delegation transaction details if success is True + :raises: ValueError for invalid validator address format + :raises: TimeoutError if balance update times out """
78-79: Consider handling additional specific exceptions.The error handling could be more granular to provide better error messages for different failure scenarios.
- except (TimeoutError, ValueError) as e: + except ValueError as e: + return {"success": False, "error": f"Validation error: {str(e)}"} + except TimeoutError as e: + return {"success": False, "error": f"Balance update timeout: {str(e)}"} + except Exception as e: + return {"success": False, "error": f"Unexpected error during compounding: {str(e)}"}
72-76: Consider adding more details to the success response.The success response could include more useful information about the compounded rewards.
return { "success": True, + "compounded_amount": str(rewards_to_stake), "withdraw_response": withdraw_response, "delegate_response": delegate_response, + "validator_address": validator_address }
81-87: Add validation for the timeout parameter.The timeout parameter should also be validated to ensure it's greater than zero and greater than the interval.
interval: int = 1 ) -> Decimal: if interval <= 0: raise ValueError("Interval must be greater than zero.") + if timeout <= 0: + raise ValueError("Timeout must be greater than zero.") + if timeout < interval: + raise ValueError("Timeout must be greater than or equal to interval.")
99-107: Consider implementing exponential backoff for polling.The current fixed interval polling might not be optimal for all network conditions. Consider implementing exponential backoff to reduce unnecessary API calls while still maintaining responsiveness.
- for _ in range(timeout // interval): + current_interval = interval + elapsed_time = 0 + while elapsed_time < timeout: balance_response = await self.chain_client.client.get_bank_balance( address=self.chain_client.address.to_acc_bech32(), denom=denom ) updated_balance = Decimal(balance_response.balance.amount) if updated_balance != old_balance: return updated_balance - await asyncio.sleep(interval) + await asyncio.sleep(current_interval) + elapsed_time += current_interval + current_interval = min(current_interval * 2, timeout - elapsed_time)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
injective_functions/staking/__init__.py(2 hunks)
🧰 Additional context used
📓 Learnings (1)
injective_functions/staking/__init__.py (2)
Learnt from: defser
PR: InjectiveLabs/iAgent#4
File: injective_functions/staking/__init__.py:41-46
Timestamp: 2024-11-25T17:25:22.648Z
Learning: In `injective_functions/utils/initializers.py`, the method `build_and_broadcast_tx` already performs transaction simulation by calling `simulate`. Therefore, when using `build_and_broadcast_tx`, it's unnecessary to perform an explicit transaction simulation beforehand.
Learnt from: defser
PR: InjectiveLabs/iAgent#4
File: injective_functions/staking/__init__.py:64-70
Timestamp: 2024-11-25T17:25:08.015Z
Learning: In `injective_functions/utils/initializers.py`, the `build_and_broadcast_tx` method already simulates transactions before execution, making additional simulation unnecessary.
🔇 Additional comments (1)
injective_functions/staking/__init__.py (1)
11-11: LGTM! Good addition of the decimal conversion constant.
The constant improves code maintainability and readability by centralizing the decimal conversion factor.
…ORMAT_DECIMALS and INJ_DENOM
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
injective_functions/staking/__init__.py (3)
27-32: Enhance docstring with return value detailsThe docstring should include information about the return value structure, particularly the success/error fields and response objects.
async def compound_rewards(self, validator_address: str) -> Dict: """ Compounds staking rewards by withdrawing them and restaking. :param validator_address: The validator's address + :return: Dict containing: + - success: bool indicating operation success + - error: str with error message if success is False + - withdraw_response: Dict with withdrawal transaction details if success is True + - delegate_response: Dict with delegation transaction details if success is True """
82-109: Consider implementing exponential backoffThe current implementation uses a fixed interval. Consider implementing exponential backoff to reduce API load while maintaining responsiveness.
async def wait_for_balance_update( self, old_balance: Decimal, denom: str, timeout: int = 10, - interval: int = 1 + base_interval: float = 0.5 ) -> Decimal: """ Waits for the balance to update after a transaction. :param old_balance: Previous balance to compare against :param denom: Denomination of the token (e.g., "inj") :param timeout: Total time to wait (in seconds) - :param interval: Time between balance checks (in seconds) + :param base_interval: Initial time between balance checks (in seconds) :return: Updated balance """ - if interval <= 0: - raise ValueError("Interval must be greater than zero.") + if base_interval <= 0: + raise ValueError("Base interval must be greater than zero.") - for _ in range(timeout // interval): + elapsed_time = 0 + current_interval = base_interval + while elapsed_time < timeout: balance_response = await self.chain_client.client.get_bank_balance( address=self.chain_client.address.to_acc_bech32(), denom=denom ) updated_balance = Decimal(balance_response.balance.amount) if updated_balance != old_balance: return updated_balance - await asyncio.sleep(interval) + await asyncio.sleep(current_interval) + elapsed_time += current_interval + current_interval = min(current_interval * 2, timeout - elapsed_time) raise TimeoutError("Balance did not update within the timeout period.")
Line range hint
20-26: Use Decimal instead of float for amountUsing float for cryptocurrency amounts can lead to precision loss. This method should be consistent with compound_rewards and use Decimal.
async def stake_tokens(self, validator_address: str, amount: str) -> Dict: # prepare tx msg msg = self.chain_client.composer.MsgDelegate( delegator_address=self.chain_client.address.to_acc_bech32(), validator_address=validator_address, - amount=float(amount), + amount=Decimal(amount) / Decimal(f"1e{ADDITIONAL_CHAIN_FORMAT_DECIMALS}"), ) return await self.chain_client.build_and_broadcast_tx(msg)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
injective_functions/staking/__init__.py(2 hunks)
🧰 Additional context used
📓 Learnings (1)
injective_functions/staking/__init__.py (2)
Learnt from: defser
PR: InjectiveLabs/iAgent#4
File: injective_functions/staking/__init__.py:41-46
Timestamp: 2024-11-25T17:25:22.648Z
Learning: In `injective_functions/utils/initializers.py`, the method `build_and_broadcast_tx` already performs transaction simulation by calling `simulate`. Therefore, when using `build_and_broadcast_tx`, it's unnecessary to perform an explicit transaction simulation beforehand.
Learnt from: defser
PR: InjectiveLabs/iAgent#4
File: injective_functions/staking/__init__.py:64-70
Timestamp: 2024-11-25T17:25:08.015Z
Learning: In `injective_functions/utils/initializers.py`, the `build_and_broadcast_tx` method already simulates transactions before execution, making additional simulation unnecessary.
🔇 Additional comments (2)
injective_functions/staking/__init__.py (2)
1-7: LGTM! Import changes are appropriate
The imports have been updated correctly to support the new async functionality and token decimal handling.
33-81: Implementation looks solid! Verify validator address existence
The implementation is well-structured with proper error handling and precise decimal calculations. Consider adding validation for validator existence.
|
@djosse please review |
|
@defser can you please rebase with master? |
Done |
This pull request introduces a new feature to the
InjectiveStakingmodule: thecompound_rewardsfunction. This function enables users to withdraw staking rewards from a validator and automatically re-stake them, compounding the rewards to maximize staking benefits.Key Changes
1. New Functionality:
compound_rewards(validator_address: str)2. Error Handling:
3. Optimizations:
wait_for_balance_updateto ensure updated balance information after withdrawals.How to Test
compound_rewardsfunction with a validvalidator_address.Benefits
Notes
wait_for_balance_updateimplementation.Checklist
compound_rewardsfunction.Feel free to suggest improvements or additional scenarios to test!
Summary by CodeRabbit
New Features
Documentation
compound_rewardsfunction, specifying required parameters.Chores
compound_rewardsfunction in staking operations.