usdsui single asset loop#59
Conversation
jangid
left a comment
There was a problem hiding this comment.
Review
+188 / -11 across 3 files — adds USDSUI single-asset looping support with reward collection and multi-hop swapping via Bluefin DEX.
Issues
1. Swap direction likely inverted for USDC → USDSUI (critical)
In collectAndSwapRewards(), the final swap step for USDSUI pools has:
typeArguments: [this.poolLabel.asset.type, this.poolLabel.asset.type, usdcCoin.coinType],This puts USDSUI as both the pool asset type AND the "from" coin, with USDC as the "to" coin. But the intent is to swap USDC → USDSUI. The getPoolIdBySymbolsAndProtocol call correctly uses ('USDC', 'USDSUI', 'bluefin'), but the type arguments and tx.pure.bool(false) for a_to_b look contradictory. Please verify against the Move contract's expected type param order — a wrong swap direction would lose user funds.
2. Hardcoded slippage tolerances with no documentation
Multiple magic numbers: 1000, 10, 10000. No comments explaining:
- What unit they're in (basis points? DEX-specific?)
- Why ALPHA→stSUI uses
1000, stSUI→SUI uses10, and everything else uses10000 - If
10000= 100% slippage, that's significant MEV risk
3. Reward coin list is hardcoded and not pool-aware
collectAndSwapRewards() fetches 10 specific coin types regardless of which rewards the pool actually earns. This means unnecessary RPC calls, manual updates when new reward tokens are added, and silent skipping of unknown reward types.
4. No error handling for missing Bluefin pools
getPoolIdBySymbolsAndProtocol / getPoolIdByTypesAndProtocol are awaited inside a loop with no handling if a Bluefin pool doesn't exist for a reward pair. This would throw and abort the entire deposit/withdraw transaction.
5. let should be const
let coinTypes = [...] is never reassigned — should be const.
6. Test file left in dry-run mode
testRun.ts has executeTransactionBlock commented out. Fine for testing, but worth calling out so it doesn't surprise anyone.
7. Empty PR description
No context on what USDSUI single-asset looping is, why reward collection happens during deposit/withdraw, or how to test.
Verdict
The core logic follows the existing SingleAssetLooping pattern and looks structurally sound. However, the USDC→USDSUI swap type arguments need verification against the Move contract before merging. The slippage values also need documentation or a confirming comment that 10000 is intentional and safe.
|
|
Since you changed poolLabel in alphafi-sdk-rust, you need to update it in parsePoolLabelEntry function in src/models/strategyContext.ts file as well |
poolLabel hasnt been changed. |
jangid
left a comment
There was a problem hiding this comment.
Re-reviewed after Shrome's changes and clarifications.
Critical concern from prior review resolved by explanation:
- The
10000/1000/10values are minimum-swap-amount, not slippage bps — no MEV risk as I'd framed it. - For the USDC→USDSUI hop, type args
[USDSUI, USDSUI, USDC]witha_to_b: falsecorrectly swaps USDC→USDSUI per the Move signature[pool_asset, A, B]. let→constdone.collectAndSwapRewardsnow also called fromclaimWithdrawandcancelWithdraw— good catch.
Remaining nice-to-haves (non-blocking, follow-up welcome):
- Add a short comment documenting that the u64 arg is min-swap-amount (and the unit), so the next reader doesn't repeat my misread.
- Pool-aware reward list and graceful handling of missing Bluefin pools can be revisited later.
LGTM.
No description provided.