Repository navigation
feat(holdings): add holdings management and investments account support - #22
Conversation
- Add SearchSecurities with configurable limit parameter - Add CreateHolding, DeleteHolding, UpdateHoldingQuantity - Add CreateHoldingByTicker convenience method - Add CreateInvestmentsAccount for manual investment accounts - Simplify GetHoldings query to use account-based holdings endpoint - Add holdings_test.go with tests for search, create, delete, update - Add CreateInvestmentsAccountParams and InitialHolding types
Cover UpdateHoldingQuantity (success, not found, get error, delete error), SearchSecurities (default limit, GraphQL error), CreateHolding (GraphQL error), DeleteHolding (GraphQL error, with errors), and CreateHoldingByTicker (search error).
|
|
||
| // UpdateHoldingQuantity updates a holding's quantity by deleting and recreating it. | ||
| // The Monarch API does not support direct quantity updates on holdings. | ||
| func (s *accountService) UpdateHoldingQuantity(ctx context.Context, accountID, holdingID string, newQuantity float64) (*Holding, error) { |
There was a problem hiding this comment.
This update path is more destructive than it needs to be. Monarch exposes a direct updateHolding(input:) mutation, and UpdateHoldingInput includes quantity, so delete+recreate can lose the holding if recreate fails, changes the holding ID, and can drop metadata like cost basis/tax lots/history. I live-smoked this on a temporary manual account and the method did return a new holding ID. I think this should call updateHolding directly instead.
There was a problem hiding this comment.
Good catch — I'll replace the delete+recreate with a direct updateHolding(input:) mutation call. This preserves the holding ID, cost basis, and history. I'll add the GraphQL mutation file and update the method.
| Name: name, | ||
| Quantity: edge.Node.Quantity, | ||
| Price: edge.Node.Price, | ||
| Price: price, |
There was a problem hiding this comment.
This only uses security.currentPrice, but the query also fetches holdings[].closingPrice and that value can be the only populated price. In the live smoke against my account, 4 returned holdings had Value > 0 but Price == 0 through this mapper. Please fall back to the sub-holding closing price, or derive from totalValue / quantity when quantity is non-zero, so Holding.Price stays useful.
There was a problem hiding this comment.
You're right. I'll add a fallback chain: use security.currentPrice first, then holdings[0].closingPrice, then derive from totalValue / quantity when quantity > 0.
| break | ||
| } | ||
| } | ||
| if securityID == "" { |
There was a problem hiding this comment.
For a mutation helper, I think this should fail closed instead of silently using the first result when there is no exact ticker match. Search results are popularity-ranked and can include adjacent/ambiguous securities; creating the wrong holding would be hard for callers to notice. Could we require an exact ticker match, probably case-normalized, and return an error otherwise?
There was a problem hiding this comment.
Agreed — fail closed is safer here. I'll require a case-insensitive exact ticker match and return an error if none is found.
- UpdateHoldingQuantity: use direct updateHolding mutation instead of destructive delete+recreate. Preserves holding ID, cost basis, and history. - GetHoldings price: add fallback chain — security.currentPrice, then holdings[0].closingPrice, then totalValue/quantity. - CreateHoldingByTicker: require case-insensitive exact ticker match, return error instead of silently using first result. - Update tests to match new behavior.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (58.16%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #22 +/- ##
==========================================
+ Coverage 42.77% 48.35% +5.57%
==========================================
Files 16 17 +1
Lines 1833 2031 +198
==========================================
+ Hits 784 982 +198
+ Misses 974 959 -15
- Partials 75 90 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
2 participants
Summary
SearchSecurities(ctx, query, limit)— searches for securities by ticker or name, with configurable result limit (defaults to 10 if<= 0)CreateHolding/DeleteHolding/UpdateHoldingQuantityfor manual investment holdingsCreateHoldingByTickerconvenience method (looks up security then creates holding)CreateInvestmentsAccountfor creating manual investment accounts with initial holdingsGetHoldingsquery to use the account-based holdings endpoint (simpler, more reliable)holdings_test.gowith tests for search, create, delete, and update operationsNo auth or transaction changes are included — this PR is self-contained.
Split from #19 per review feedback.
Test plan
go test ./...)SearchSecurities("BTC", 5)returns bitcoin-related securitiesCreateHoldingByTicker(accountID, "BTC", 1.5)creates a BTC holdingDeleteHolding(holdingID)removes a holdingUpdateHoldingQuantity(accountID, holdingID, 2.0)updates quantityCreateInvestmentsAccountcreates account with initial holdings