Conversation
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com> # Conflicts: # poetry.lock
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds an internal CLI command to process the OsToken redeemer (exit-queue checkpoints + automatic redemption execution), while generalizing harvest/withdrawable-asset helpers to work with arbitrary vault addresses (needed for multi-vault/meta-vault flows).
Changes:
- Parameterize
get_harvest_paramsandget_withdrawable_assetswith an explicitvaultaddress and update call sites accordingly. - Introduce
process_redeemerinternal command to process exit queue and redeem eligible OsToken positions (with multiproof + multicall). - Skip redemption-position iteration when
merkle_rootis zero (no active set) and extend typings to trackavailable_shares/shares_to_redeem.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/withdrawals/assets.py | Updates harvest-param fetch to pass explicit vault address. |
| src/validators/tasks.py | Updates harvest/withdrawable calls to new vault-parameterized signatures. |
| src/validators/execution.py | Makes get_withdrawable_assets accept a vault argument. |
| src/reward_splitter/tasks.py | Updates harvest-param fetch to pass explicit vault address. |
| src/redemptions/typings.py | Extends OsTokenPosition with redemption-tracking fields. |
| src/redemptions/tasks.py | Adds ZERO_MERKLE_ROOT guard to skip empty redeemable sets. |
| src/main.py | Registers the new internal process_redeemer CLI command. |
| src/harvest/tasks.py | Updates harvest-param fetch to pass explicit vault address. |
| src/common/startup_check.py | Updates harvest/withdrawable calls to new vault-parameterized signatures. |
| src/common/harvest.py | Makes get_harvest_params accept a vault argument. |
| src/common/contracts.py | Adds OsToken redeemer wrapper methods (queued shares, exit queue processing, sub-vault redemption, etc.). |
| src/commands/internal/process_redeemer.py | New command implementing redemption selection + multiproof redemption + exit queue processing. |
| src/commands/tests/test_internal/test_process_redeemer.py | New unit tests for the redeemer processing logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| vault_positions_shares = Wei(sum(p.available_shares for p in positions)) | ||
| vault_positions_assets = os_token_converter.to_assets(vault_positions_shares) | ||
|
|
||
| if vault_positions_assets <= withdrawable_assets or not await is_meta_vault(vault_address): | ||
| return withdrawable_assets | ||
|
|
||
| logger.info('Vault %s is a meta-vault with insufficient withdrawable assets.', vault_address) | ||
| additional_assets_needed = Wei(vault_positions_assets - withdrawable_assets) |
There was a problem hiding this comment.
_try_redeem_sub_vaults decides how much to redeem from sub-vaults using the sum of all available_shares in the vault. Since the overall redemption in this run is capped by queued_shares, this can trigger an on-chain redeemSubVaultsAssets for assets that will never be redeemed in this round (e.g., when queued_shares is smaller than total vault position shares or gets exhausted on earlier vaults). Consider limiting the required shares/assets to the maximum that could actually be redeemed for this vault in the current round (based on remaining queued shares / selection), or move sub-vault redemption to after positions are selected.
| vault_positions_shares = Wei(sum(p.available_shares for p in positions)) | |
| vault_positions_assets = os_token_converter.to_assets(vault_positions_shares) | |
| if vault_positions_assets <= withdrawable_assets or not await is_meta_vault(vault_address): | |
| return withdrawable_assets | |
| logger.info('Vault %s is a meta-vault with insufficient withdrawable assets.', vault_address) | |
| additional_assets_needed = Wei(vault_positions_assets - withdrawable_assets) | |
| # Use the shares that are actually intended to be redeemed in this round for this vault. | |
| # Prefer `shares_to_redeem` when present; fall back to `available_shares` otherwise. | |
| requested_shares = Wei( | |
| sum( | |
| (getattr(p, 'shares_to_redeem', None) or p.available_shares) | |
| for p in positions | |
| ) | |
| ) | |
| if requested_shares == 0: | |
| # Nothing to redeem for this vault in the current round. | |
| return withdrawable_assets | |
| requested_assets = os_token_converter.to_assets(requested_shares) | |
| if requested_assets <= withdrawable_assets or not await is_meta_vault(vault_address): | |
| return withdrawable_assets | |
| logger.info('Vault %s is a meta-vault with insufficient withdrawable assets.', vault_address) | |
| additional_assets_needed = Wei(requested_assets - withdrawable_assets) |
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/common/harvest.py
Outdated
| last_rewards = await keeper_contract.get_last_rewards_update(block_number) | ||
| if last_rewards is None: | ||
| return None | ||
| return {vault: None for vault in vaults} | ||
|
|
||
| ipfs_data = await ipfs_fetch_client.fetch_json(last_rewards.ipfs_hash) | ||
|
|
||
| for vault in vaults: | ||
| if not await keeper_contract.can_harvest(vault, block_number): | ||
| results[vault] = None |
There was a problem hiding this comment.
get_multiple_harvest_params now fetches IPFS data (ipfs_fetch_client.fetch_json) before checking can_harvest for any vaults. This regresses the previous behavior where non-harvestable vaults would return early without an IPFS fetch, and can now cause unnecessary IPFS load or even crash the caller when IPFS is unavailable despite can_harvest being false. Consider checking can_harvest first (for all vaults) and only fetching IPFS data if at least one vault is harvestable, or restoring the early-return behavior in get_harvest_params for the single-vault case.
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Get harvest params for multiple vaults. | ||
|
|
||
| Checks can_harvest for all vaults first, then fetches IPFS data only | ||
| if at least one vault is harvestable. | ||
| """ | ||
| if not vaults: | ||
| return {} | ||
|
|
||
| last_rewards = await keeper_contract.get_last_rewards_update(block_number) | ||
| if last_rewards is None: | ||
| return None | ||
| return {vault: None for vault in vaults} |
There was a problem hiding this comment.
The docstring says can_harvest is checked for all vaults before fetching remote data, but the implementation calls keeper_contract.get_last_rewards_update() first. This adds an extra (potentially expensive) chain log lookup even when no vault is harvestable. Consider checking can_harvest for the provided vaults first and only then fetching last rewards/IPFS (or update the docstring if the order is intentional).
Signed-off-by: cyc60 <avsysoev60@gmail.com>
evgeny-stakewise
left a comment
There was a problem hiding this comment.
Added comments part 2
|
|
||
| logger.info('Vault %s is a meta-vault with insufficient withdrawable assets.', vault_address) | ||
| try: | ||
| tx_hash = await os_token_redeemer_contract.redeem_sub_vaults_assets(vault_address, deficit) |
There was a problem hiding this comment.
Probably you have to redeem nested meta vaults as well.
There was a problem hiding this comment.
nested meta vaults handled in contact code
Signed-off-by: cyc60 <avsysoev60@gmail.com>
Signed-off-by: cyc60 <avsysoev60@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if queued_assets < min_queued_assets: | ||
| logger.info( | ||
| 'Queued assets %s below threshold %s. Skipping to next interval.', | ||
| Web3.from_wei(queued_assets, 'ether'), | ||
| Web3.from_wei(min_queued_assets, 'ether'), | ||
| ) |
There was a problem hiding this comment.
min_queued_assets is parsed as a Gwei value (see main(... min_queued_assets=Gwei(min_queued_assets_gwei))), but here it's treated as Wei: queued_assets is in Wei and Web3.from_wei(min_queued_assets, 'ether') also expects Wei. This makes the threshold comparison/logging incorrect by 1e9. Convert min_queued_assets_gwei to Wei once (e.g., min_queued_assets_wei = Web3.to_wei(..., 'gwei')) and consistently compare/log in Wei units.
| last_rewards = await keeper_contract.get_last_rewards_update(block_number) | ||
| if last_rewards is None: | ||
| return None | ||
| return {vault: None for vault in vaults} | ||
|
|
||
| harvestable_vaults: list[ChecksumAddress] = [ | ||
| vault for vault in vaults if await keeper_contract.can_harvest(vault, block_number) | ||
| ] | ||
|
|
||
| if not harvestable_vaults: | ||
| return {vault: None for vault in vaults} | ||
|
|
There was a problem hiding this comment.
get_multiple_harvest_params calls keeper_contract.can_harvest(...) sequentially for each vault (await inside the list comprehension). For many vaults (as in the new redeemer flow) this adds avoidable latency. Consider issuing these calls concurrently with asyncio.gather and then filtering based on the results.
Signed-off-by: cyc60 <avsysoev60@gmail.com>
No description provided.