-
Notifications
You must be signed in to change notification settings - Fork 198
feat: multi-account yield support #11665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
- Replace useEffect with useMemo for default account selection in YieldAccountContext (per CLAUDE.md guidelines) - Extract duplicate URL sync logic to useYieldAccountSync hook - Fix aggregate calculation in YieldOpportunityStats to sum all active/locked balances across accounts (was using find instead of filter) - Ensure feature flag properly gates single-account filtering - Fix type inconsistency in useYieldAccount hook (return 0 not undefined) - Remove duplicate account column in YieldActivePositions table - Add comprehensive tests for yield utils Co-Authored-By: Claude Opus 4.5 <[email protected]>
…selector - YieldSuccess now accepts accountId and includes it in "View Position" navigation URL - Updated all YieldSuccess callers (YieldForm, YieldEnterModal, YieldActionModal) - Fixed YieldEnterModal to fallback to first available account when no accountId prop - Show validator name/logo for staking positions in YieldActivePositions table - Route to yield detail with accountId in query params when clicking positions Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
📝 WalkthroughWalkthroughAdds multi-account support for yield flows: feature flag, account context with URL sync, account-scoped balance queries, account selector UI, plumbing of optional Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Yield UI (Page/Components)
participant Hook as useYieldAccountSync
participant Context as YieldAccountContext
participant Query as useAllYieldBalances
participant API as Yield API
User->>UI: Select account in AccountSelector
UI->>Hook: handleAccountChange(newAccountId)
Hook->>Context: setAccountId(newAccountId)
Hook->>UI: update URL ?accountId=newAccountId
Context-->>UI: updated accountId
UI->>Query: fetch balances(targetAccountIds=[newAccountId])
Query->>API: GET /balances?accountIds=...
API-->>Query: balances data
Query-->>UI: balances response
UI-->>User: render positions & stats for selected account
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (6)**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{tsx,jsx}📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
Files:
**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/naming-conventions.mdc)
Files:
**/*.{jsx,tsx}📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
Files:
**/*.tsx📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
Files:
🧠 Learnings (16)📓 Common learnings📚 Learning: 2025-09-08T15:53:09.362ZApplied to files:
📚 Learning: 2025-11-24T21:20:04.979ZApplied to files:
📚 Learning: 2025-08-15T07:51:16.374ZApplied to files:
📚 Learning: 2025-11-19T16:59:50.569ZApplied to files:
📚 Learning: 2025-07-29T15:04:28.083ZApplied to files:
📚 Learning: 2025-10-21T17:11:18.087ZApplied to files:
📚 Learning: 2025-08-22T12:59:01.702ZApplied to files:
📚 Learning: 2025-08-22T12:58:26.590ZApplied to files:
📚 Learning: 2025-07-24T11:07:20.536ZApplied to files:
📚 Learning: 2025-08-22T12:58:36.070ZApplied to files:
📚 Learning: 2026-01-07T15:36:13.236ZApplied to files:
📚 Learning: 2025-08-13T15:52:25.116ZApplied to files:
📚 Learning: 2025-08-10T21:09:25.643ZApplied to files:
📚 Learning: 2025-12-27T16:02:52.792ZApplied to files:
📚 Learning: 2026-01-15T09:59:33.508ZApplied to files:
🧬 Code graph analysis (1)src/pages/Yields/components/YieldPositionCard.tsx (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/pages/Yields/components/YieldPositionCard.tsx`:
- Line 63: The guard "if (accountNumber === undefined) return undefined" in
YieldPositionCard is unreachable because useYieldAccount always returns
accountNumber as a number (defaults to 0); remove this conditional and any early
return that depends on accountNumber being undefined to simplify the component
and rely on the numeric value from useYieldAccount instead.
🧹 Nitpick comments (3)
src/pages/Yields/components/YieldActionModal.tsx (1)
46-46: UseAccountIdtype instead ofstringfor type consistency.The
accountIdprop is typed asstring, butYieldSuccessand other components in this PR useAccountIdfrom@shapeshiftoss/caip. Using the nominalAccountIdtype ensures type safety across the yield flow.Suggested fix
Add the import at the top of the file:
import type { AccountId } from '@shapeshiftoss/caip'Then update the prop type:
type YieldActionModalProps = { isOpen: boolean onClose: () => void yieldItem: AugmentedYieldDto action: 'enter' | 'exit' | 'manage' amount: string assetSymbol: string assetLogoURI?: string validatorAddress?: string validatorName?: string validatorLogoURI?: string passthrough?: string manageActionType?: string - accountId?: string + accountId?: AccountId }src/pages/Yields/YieldAssetDetails.tsx (1)
115-118: Minor inconsistency with YieldDetail.tsx balance fetching logic.In
YieldDetail.tsx(line 68-71), whenisYieldMultiAccountEnabledis false,balanceAccountIdsreturnsundefinedifavailableAccountsis empty. Here, it returnsaccountIdsForAssetdirectly (potentially an empty array).This difference may cause inconsistent behavior in
useAllYieldBalancesdepending on how it handles empty arrays vs.undefined. Consider aligning the logic:const balanceAccountIds = useMemo(() => { - if (!isYieldMultiAccountEnabled) return accountIdsForAsset + if (!isYieldMultiAccountEnabled) return accountIdsForAsset.length > 0 ? accountIdsForAsset : undefined return selectedAccountId ? [selectedAccountId] : accountIdsForAsset }, [isYieldMultiAccountEnabled, selectedAccountId, accountIdsForAsset])src/pages/Yields/components/YieldActivePositions.tsx (1)
87-141: Validator info may be lost when aggregating multiple validators.When a user has positions with multiple validators in the same yield, only the first validator's info is preserved due to the
existing.validatorAddress ?? validator?.addresspattern. If the intent is to show separate positions per validator, consider grouping by(accountId, validatorAddress)instead of justaccountId.If showing one aggregate per account is intentional (with "first validator wins"), this is acceptable but worth documenting.
- Remove toUserCurrency utility (use inline bnOrZero().times().toFixed() pattern) - Remove trivial lookup table tests for getTransactionButtonText - Keep meaningful tests for edge cases, fallbacks, and actual logic Co-Authored-By: Claude Opus 4.5 <[email protected]>
accountNumber always returns a number (defaults to 0) from useYieldAccount, so the undefined check is unreachable. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Description
Does what it says on the box i.e:
Issue (if applicable)
Risk
Low - behind
YieldMultiAccountfeature flagNone - UI only changes for yield display and account selection
Testing
Engineering
Note: test with flag on - assume this will ship with it
Operations
Screenshots (if applicable)
https://jam.dev/c/fbf0f785-b277-424e-8f07-20d4ce900339
Summary by CodeRabbit
New Features
UI Improvements
Translations
Tests
✏️ Tip: You can customize this high-level summary in your review settings.