Conversation
|
Review changes with Changed Files
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new yielder crate for handling yield-providing protocols, starting with an integration for "Yo". The new crate is well-structured with clear separation of concerns for providers, clients, and contracts. The integration into GemGateway replaces placeholder implementations with calls to the new Yielder instance.
My review focuses on improving error handling. Specifically, I've pointed out a few places where errors are silently ignored, which could make debugging difficult. I've also suggested a way to preserve more detailed error information when propagating errors from the yielder crate up to the GemGateway.
crates/yielder/src/yielder.rs
Outdated
| .iter() | ||
| .filter_map(|asset| { | ||
| let chain = asset.chain; | ||
| let client = create_eth_client(rpc_provider.clone(), chain).ok()?; |
There was a problem hiding this comment.
The use of .ok()? silently discards errors from create_eth_client. This can hide configuration problems, such as a missing RPC endpoint for a supported chain. It would be beneficial to log these errors to aid in debugging.
crates/yielder/src/yielder.rs
Outdated
|
|
||
| pub async fn positions(&self, chain: Chain, address: &str, asset_ids: &[AssetId]) -> Vec<DelegationBase> { | ||
| let futures: Vec<_> = self.providers.iter().map(|p| p.get_positions(chain, address, asset_ids)).collect(); | ||
| futures::future::join_all(futures).await.into_iter().filter_map(|r| r.ok()).flatten().collect() |
There was a problem hiding this comment.
Errors from individual providers are silently ignored here by using .ok(). While this makes the function more robust if one provider fails, it also hides potential issues. Consider logging the errors to provide visibility into provider failures.
gemstone/src/gateway/mod.rs
Outdated
| msg: "Earn provider not available".to_string(), | ||
| }) | ||
| pub async fn get_earn_data(&self, asset_id: AssetId, address: String, value: String, earn_type: GemEarnType) -> Result<GemContractCallData, GatewayError> { | ||
| self.yielder.get_earn_data(&asset_id, &address, &value, &earn_type).await.map_err(|e| GatewayError::NetworkError { msg: e.to_string() }) |
There was a problem hiding this comment.
Converting all YielderError variants to a generic GatewayError::NetworkError with a string message loses valuable structured error information. This can make error handling on the client side less precise. Consider adding a more specific error variant to GatewayError for yielder-related failures, or matching on the YielderError to map it to different existing GatewayError variants. For example, YielderError::UnsupportedAsset could be mapped to a more specific error than a generic network error.
crates/yielder/src/yo/assets.rs
Outdated
|
|
||
| pub const YO_USDC: YoAsset = YoAsset { | ||
| chain: Chain::Base, | ||
| asset_token: address!("0x833589fcd6edb6e08f4c7c32d4f71b54bda02913"), |
crates/yielder/src/yo/client.rs
Outdated
| .collect() | ||
| } | ||
|
|
||
| async fn check_token_allowance(&self, token: Address, owner: Address, amount: U256) -> Result<Option<ApprovalData>, YielderError> { |
There was a problem hiding this comment.
we already have this method somewhere else
crates/yielder/src/yo/client.rs
Outdated
| .collect() | ||
| } | ||
|
|
||
| async fn check_token_allowance(&self, token: Address, owner: Address, amount: U256) -> Result<Option<ApprovalData>, YielderError> { |
There was a problem hiding this comment.
no need to use multicall and check for reuse
crates/yielder/src/yo/provider.rs
Outdated
|
|
||
| use super::{YO_PARTNER_ID_GEM, YoAsset, client::YoClient, supported_assets}; | ||
|
|
||
| const GAS_LIMIT: &str = "300000"; |
There was a problem hiding this comment.
| const GAS_LIMIT: &str = "300000"; | |
| const GAS_LIMIT: u64= 300_000; |
crates/yielder/src/yo/provider.rs
Outdated
| return None; | ||
| } | ||
| let asset_id = a.asset_id(); | ||
| Some(DelegationBase { |
There was a problem hiding this comment.
move to mapper.rs. map_to_delegation
crates/yielder/src/yo/provider.rs
Outdated
| Ok(assets | ||
| .iter() | ||
| .zip(positions) | ||
| .filter_map(|(a, data)| { |
There was a problem hiding this comment.
| .filter_map(|(a, data)| { | |
| .filter_map(|(asset, data)| { |
crates/yielder/src/yielder.rs
Outdated
| self.providers.iter().flat_map(|p| p.earn_providers(asset_id)).collect() | ||
| } | ||
|
|
||
| pub async fn positions(&self, chain: Chain, address: &str, asset_ids: &[AssetId]) -> Vec<DelegationBase> { |
There was a problem hiding this comment.
| pub async fn positions(&self, chain: Chain, address: &str, asset_ids: &[AssetId]) -> Vec<DelegationBase> { | |
| pub async fn get_positions(&self, chain: Chain, address: &str, asset_ids: &[AssetId]) -> Vec<DelegationBase> { |
crates/yielder/src/yielder.rs
Outdated
| futures::future::join_all(futures).await.into_iter().filter_map(|r| r.ok()).flatten().collect() | ||
| } | ||
|
|
||
| pub async fn balance(&self, chain: Chain, address: &str) -> Vec<AssetBalance> { |
There was a problem hiding this comment.
| pub async fn balance(&self, chain: Chain, address: &str) -> Vec<AssetBalance> { | |
| pub async fn get_balance(&self, chain: Chain, address: &str) -> Vec<AssetBalance> { |
gemstone/src/gateway/mod.rs
Outdated
| msg: "Earn provider not available".to_string(), | ||
| }) | ||
| pub async fn get_earn_data(&self, asset_id: AssetId, address: String, value: String, earn_type: GemEarnType) -> Result<GemContractCallData, GatewayError> { | ||
| self.yielder.earn_data(&asset_id, &address, &value, &earn_type).await.map_err(|e| GatewayError::NetworkError { msg: e.to_string() }) |
There was a problem hiding this comment.
| self.yielder.earn_data(&asset_id, &address, &value, &earn_type).await.map_err(|e| GatewayError::NetworkError { msg: e.to_string() }) | |
| self.yielder.get_earn_data(&asset_id, &address, &value, &earn_type).await.map_err(|e| GatewayError::NetworkError { msg: e.to_string() }) |
crates/yielder/src/provider.rs
Outdated
| fn earn_providers(&self, asset_id: &AssetId) -> Vec<DelegationValidator>; | ||
| fn earn_asset_ids_for_chain(&self, chain: Chain) -> Vec<AssetId>; | ||
|
|
||
| async fn positions(&self, chain: Chain, address: &str, asset_ids: &[AssetId]) -> Result<Vec<DelegationBase>, YielderError>; |
There was a problem hiding this comment.
| async fn positions(&self, chain: Chain, address: &str, asset_ids: &[AssetId]) -> Result<Vec<DelegationBase>, YielderError>; | |
| async fn positions(&self, asset_id: &AssetId, address: &str) -> Result<Vec<DelegationBase>, YielderError>; |
crates/yielder/src/provider.rs
Outdated
| pub trait EarnProvider: Send + Sync { | ||
| fn id(&self) -> YieldProvider; | ||
| fn earn_providers(&self, asset_id: &AssetId) -> Vec<DelegationValidator>; | ||
| fn earn_asset_ids_for_chain(&self, chain: Chain) -> Vec<AssetId>; |
There was a problem hiding this comment.
| fn earn_asset_ids_for_chain(&self, chain: Chain) -> Vec<AssetId>; |
crates/yielder/src/yo/provider.rs
Outdated
| } | ||
|
|
||
| async fn get_balance(&self, chain: Chain, address: &str, token_ids: &[String]) -> Result<Vec<AssetBalance>, YielderError> { | ||
| let token_match = |a: &&YoAsset| token_ids.iter().any(|t| a.asset_token.to_string().eq_ignore_ascii_case(t)); |
There was a problem hiding this comment.
| let token_match = |a: &&YoAsset| token_ids.iter().any(|t| a.asset_token.to_string().eq_ignore_ascii_case(t)); | |
| let token_match = |a: &&YoAsset| token_ids.iter().any(|t| a.asset_token.to_string() == t); |
| self.get_position_for_asset(address, &asset).await | ||
| } | ||
|
|
||
| async fn get_balance(&self, chain: Chain, address: &str, token_ids: &[String]) -> Result<Vec<AssetBalance>, YielderError> { |
There was a problem hiding this comment.
| async fn get_balance(&self, chain: Chain, address: &str, token_ids: &[String]) -> Result<Vec<AssetBalance>, YielderError> { | |
| async fn get_balance(&self, chain: Chain, address: &str, token_id: String) -> Result<Vec<AssetBalance>, YielderError> { |
crates/yielder/src/yielder.rs
Outdated
|
|
||
| pub async fn get_data(&self, asset_id: &AssetId, address: &str, value: &str, earn_type: &EarnType) -> Result<ContractCallData, YielderError> { | ||
| let provider_id = earn_type.provider_id(); | ||
| let provider = self.providers.iter().find(|p| p.get_provider(asset_id).is_some_and(|v| v.id == provider_id)).ok_or_else(|| YielderError::unsupported_asset(asset_id))?; |
gemstone/src/gateway/mod.rs
Outdated
| #[uniffi::constructor] | ||
| pub fn new(provider: Arc<dyn AlienProvider>, preferences: Arc<dyn GemPreferences>, secure_preferences: Arc<dyn GemPreferences>, api_url: String) -> Self { | ||
| let api_client = GemApiClient::new(api_url, provider.clone()); | ||
| let wrapper = AlienProviderWrapper { provider: provider.clone() }; |
There was a problem hiding this comment.
| let wrapper = AlienProviderWrapper { provider: provider.clone() }; | |
| let wrapper = AlienProviderWrapper::new(provider.clone()); |
crates/gem_evm/src/rpc/client.rs
Outdated
| } | ||
|
|
||
| #[cfg(feature = "rpc")] | ||
| pub async fn multicall3_batch<T, R, const N: usize>( |
There was a problem hiding this comment.
| pub async fn multicall3_batch<T, R, const N: usize>( | |
| pub async fn multicall3<T, R, const N: usize>( |
|
|
||
| use super::EthereumClient; | ||
|
|
||
| pub fn create_eth_client(provider: Arc<dyn RpcProvider>, chain: Chain) -> Option<EthereumClient<RpcClient>> { |
There was a problem hiding this comment.
| pub fn create_eth_client(provider: Arc<dyn RpcProvider>, chain: Chain) -> Option<EthereumClient<RpcClient>> { | |
| pub fn create_eth_client(provider: Arc<dyn RpcProvider>, chain: EVMChain) -> EthereumClient<RpcClient> { |
Replace internal call-data helpers with YoClient impl that builds TransactionObject directly (using IYoGateway::...abi_encode). Simplify IYoGateway ABI by removing several unused view functions. Update provider logic: remove empty-assets early return in fetch_positions, inline asset filtering, adjust DelegationBase field ordering (set delegation_id and validator_id earlier), and rename local tx -> transaction while mapping .to/.data into ContractCallData. Miscellaneous formatting/cleanup.
Add #[cfg(test)] modules with unit tests for convert_to_assets_ceil (crates/yielder/src/yo/client.rs) and apply_slippage (crates/yielder/src/yo/provider.rs). Tests cover typical cases and edge cases (including zero values) to ensure correct rounding behavior and slippage calculation.
Refactor method names across the yielder and yo modules for clearer, consistent naming. Changes include: - Provider/EarnProvider: get_positions -> positions - Yielder: get_positions -> positions, get_earn_data -> earn_data, get_provider -> provider_by_id - YoClient/YoGatewayClient: get_positions_batch -> positions_batch, convert_to_shares -> quote_shares - YoEarnProvider: get_asset -> asset, fetch_positions -> positions_for_chain, get_positions -> positions - Updated all call sites (crates/yielder and gemstone gateway) to use the new names. No behavior changes intended — purely a rename refactor to improve readability and naming consistency.
…ching Add two methods to EthereumClient: - call_contract: typed eth_call for single contract calls (no multicall3 overhead) - multicall3_batch: batched multicall3 with const generic chunk size Simplify Yo client: - positions_batch uses multicall3_batch instead of manual call building + chunk decoding - check_token_allowance and quote_shares use direct call_contract instead of multicall3 - Remove duplicate IYoVaultToken (was just IERC4626 with a different name)
…tency Move apply_slippage_in_bp from swapper to gem_evm::slippage so both swapper and yielder reuse the same implementation. Yielder naming cleanup: - gateways → clients, gateway_for_chain → get_client - positions_batch → get_positions, quote_shares → get_quote_shares - asset → get_asset, positions_for_chain → get_positions_for_chain - Consistent get_ prefix across all lookup methods Remove duplicate slippage logic from yielder provider.
- Simplify EarnProvider trait: merge deposit/withdraw into single get_data, singularize get_providers/get_positions to return Option, remove id(), remove get_asset_ids_for_chain by moving get_balance into trait - Remove redundant chain param from get_position — derived from asset_id - Make Yielder a thin orchestrator: pure routing, no business logic, no provider_by_id - Move client creation from Yielder::new into YoEarnProvider::new - Hide yo/ internals — only YoEarnProvider is exported - Remove dead code: strum dependency, YieldProvider re-export, provider_not_found, strum::ParseError impl - Use biguint_to_u256 instead of BigUint → String → U256 roundtrip - Update gateway FFI: get_earn_provider (singular), get_earn_position (singular, no chain param)
…m_evm Move AlienError, RpcClient, RpcProvider, and create_client to gem_jsonrpc::alien. Add create_eth_client to gem_evm::rpc for EVM-specific client creation. Both swapper and yielder can now use shared concrete types with no generics.
Remove swapper's local AlienError definition, use gem_jsonrpc::alien instead. Update client_factory to delegate to shared create_client and create_eth_client.
EarnProvider trait: merge deposit/withdraw into get_data, singularize get_provider/get_position to Option, move get_balance into trait, remove id() and get_asset_ids_for_chain. Yielder orchestrator: pure routing with no business logic, aggregates providers/positions into Vec, deduplicates balances across providers, routes get_data by provider_id from EarnType. YoEarnProvider: drop YoClient trait and HashMap<Chain, Arc<dyn YoClient>>, use concrete YoGatewayClient created on demand via shared create_eth_client. Batch all chain assets in single multicall3 for get_balance. Cleanup: remove strum dependency, remove client_factory (use gem_evm shared), use concrete alien types (no generics), hardcode Yo APR at 4.9%, use biguint_to_u256 instead of string roundtrip, use .parse() over FromStr. Gateway FFI: get_earn_providers/get_earn_positions take single asset_id instead of chain + Vec<AssetId>.
Change get_balance(chain, address) to get_balance(chain, address, token_ids). Provider matches user's token IDs against supported earn assets with case-insensitive comparison. No matching tokens → early return, zero RPC calls.
- Extract mapping functions to mapper.rs (map_to_asset_balance, map_to_contract_call_data) - Add get_positions helper to reduce duplication in YoEarnProvider - Reuse get_asset in get_provider to avoid duplicate find logic - Remove hardcoded YO_APR, use 0.0 (iOS reads APR from DB) - Fix token comparison to use direct equality instead of case-insensitive - Add tests for map_to_asset_balance and map_to_contract_call_data
- Delete gem_evm client_factory (returned Option causing double conversion) - Inline EVM client creation in swapper with proper Result types - Add yielder/src/client_factory.rs mirroring swapper pattern - Add YielderError::NotSupportedChain unit variant - Fix import grouping in yo/provider.rs and yo/mapper.rs
- Add Yielder::with_providers for testability without real RPC - Extract get_provider(asset_id, provider_id) routing in Yielder::get_data - Add YielderError::NotSupportedAsset unit variant - Export EarnProvider trait from yielder crate - Add AlienProviderWrapper::new constructor, make field private
gemdev111
force-pushed
the
yo-yielder-intergration
branch
from
7ac0bfb to
a3662e3
Compare
2 participants
No description provided.