Skip to content

data: add Account & Wallet sur, data model doc#26

Draft
tomholford wants to merge 7 commits into
masterfrom
t/wallet-state-migration
Draft

data: add Account & Wallet sur, data model doc#26
tomholford wants to merge 7 commits into
masterfrom
t/wallet-state-migration

Conversation

@tomholford
Copy link
Copy Markdown
Owner

@tomholford tomholford commented Jan 29, 2023

Summary

Add Wallet and Account data models, creating a hierarchy: Wallet → Account → Transaction.

  • New sur/, lib/, mar/ files for Account and Wallet types (CRUD actions, JSON serialization)
  • Transaction gains account-id foreign key
  • State migration from state-0 to state-1 (adds acts and wlts maps, backfills account-id)
  • Agent restructured to use helper core pattern (=< with |_)
  • Scry endpoints for all three models (/accounts, /transactions, /wallets)
  • Subscription facts emitted on /updates for all poke operations (add/edit/del per model)
  • sur/transaction-0.hoon preserves old types for migration
  • notes/data.md documents the data model

Fixes applied

  • Removed incorrect ^- json casts from dejs arms in wallet/transaction libs (ot returns a gate)
  • Fixed wallet enjs to pass +.upd instead of full tagged update
  • Fixed account enjs @tas@t aura mismatch
  • Inlined old transaction type in agent (type path through sur import chain was unresolvable)
  • Fixed ~(run by) migration gate to accept value only
  • Fixed :* tuple construction in state migration
  • Changed helper core poke handlers to return (quip card _state) for =^ compatibility
  • Extract nested structs before field access (Hoon face resolution doesn't descend through named faces)

Test plan

  • All Hoon files parse clean (hoon-parser)
  • Desk compiles on fake ~dev ship (|commit %hodl)
  • Agent installs and boots (gall: booted %hodl)
  • Wallet pokes: add, edit, delete
  • Account pokes: add, delete
  • Transaction pokes: add, edit, delete
  • Scry endpoints return correct data
  • Subscription facts emitted without errors on all poke operations
  • Verify state migration with existing state-0 data on a real ship
  • Frontend: update poke marks from hodl-action to transaction-action (pre-existing mismatch)

Test commands (dojo on fake ~dev)

Ship setup

# Boot fresh fake ship in tmux
rm -rf /tmp/hodl-test-ship
tmux new-session -d -s urbit "$URBIT_BIN -F dev -c /tmp/hodl-test-ship"
sleep 90

# Create desk, mount, copy files
tmux send-keys -t urbit "|merge %hodl our %base" Enter; sleep 5
tmux send-keys -t urbit "|mount %hodl" Enter; sleep 5

rsync -avqL desk/* /tmp/hodl-test-ship/hodl/
# Fix sys.kelvin to include kernel version (e.g. [%zuse 409] through [%zuse 417])
rm -f /tmp/hodl-test-ship/hodl/desk.docket-0

tmux send-keys -t urbit "|commit %hodl" Enter; sleep 30
tmux send-keys -t urbit "|install our %hodl" Enter

CRUD sequence

:: Wallet
:hodl &wallet-action [%add [id='w1' name='Coinbase' note='main exchange']]
.^(* %gx /=hodl=/wallets/noun)
.^(* %gx /=hodl=/wallets/'w1'/noun)

:: Account
:hodl &account-action [%add id='a1' wallet-id='w1' name='BTC Spot' note='btc spot']
.^(* %gx /=hodl=/accounts/'a1'/noun)

:: Transaction
:hodl &transaction-action [%add [id='tx1' coin-id='bitcoin' date=now note='bought btc' amount='0.5' cost-basis='30000' type=%buy account-id='a1']]
.^(* %gx /=hodl=/transactions/noun)

:: Edits
:hodl &wallet-action [%edit [id='w1' name='Coinbase Pro' note='upgraded']]
:hodl &transaction-action [%edit [id='tx1' coin-id='bitcoin' date=now note='edited' amount='1.0' cost-basis='29000' type=%buy account-id='a1']]

:: Deletes
:hodl &wallet-action [%add [id='w2' name='Temp' note='delete me']]
:hodl &wallet-action [%del id='w2']
.^(* %gx /=hodl=/wallets/'w2'/noun)       :: expect [~ ~]
:hodl &account-action [%add id='a2' wallet-id='w1' name='Temp' note='']
:hodl &account-action [%del id='a2']
:hodl &transaction-action [%del id='tx1']
.^(* %gx /=hodl=/transactions/noun)        :: expect empty

@tomholford tomholford force-pushed the t/wallet-state-migration branch from 204daf2 to e01a235 Compare February 5, 2023 05:16
@tomholford tomholford force-pushed the t/wallet-state-migration branch from ca3f299 to 4baa333 Compare April 22, 2023 21:00
@tomholford tomholford force-pushed the t/wallet-state-migration branch from 4baa333 to 366b5a8 Compare April 24, 2023 15:30
- Remove incorrect `^- json` casts from dejs arms in wallet.hoon and
  transaction.hoon (ot returns a gate, not json)
- Fix enjs wallet update to pass `+.upd` instead of full tagged update
- Fix account.hoon acjs aura from @tas to @t to match sur definitions
Migration (on-load):
- Inline old transaction type as txn-0 to avoid unresolvable
  type path through transaction-0 sur import chain
- Fix ~(run by) gate to accept value only, not [key value]
- Fix :* tuple construction (bare faces parsed as separate elements)
- Use '' (empty cord) instead of ~ for default account-id

Helper core (poke handlers):
- Return (quip card _state) instead of _state for =^ compat
- Extract nested struct from action before accessing fields
  (txn.act/wllt.act) since face resolution doesn't descend
  through named faces like =txn or =wllt
- Add per-operation update variants to sur types ([%add ...], [%edit ...], [%del =id])
- Add JSON encoders for new update variants in lib enjs cores
- Emit %give %fact cards on /updates path from all poke handlers
- Matches existing frontend TransactionUpdate interface shape
- Align types with backend sur (Wallet, Account, Transaction + account-id)
- Add Zustand stores for wallets and accounts (poke/scry/handleUpdate)
- Refactor transaction store: hodl-action → transaction-action, /transactions/all → /transactions
- Centralize subscription in bootstrap.ts (single /updates sub, dispatch by mark)
- Update views: account-id selector on tx form, parseFloat for string amounts
- Replace old React Context stores with Zustand, delete store/Wallets + store/Accounts
- Add /wallets and /accounts routes
@tomholford
Copy link
Copy Markdown
Owner Author

Code Review

Overall Assessment

The architecture is sound and the migration approach is clean. The main concern is how the frontend subscription dispatches by mark.

Recommendation: REQUEST CHANGES


Code Quality & Patterns

Type File Observation Suggestion
Bug risk ui/src/state/bootstrap.ts event: (data: any, mark: string)@urbit/http-api's subscription handler signature is event: (data) => void. mark is not a second argument, so mark will always be undefined and every event hits default: console.warn(...). All subscription updates silently fail. Verify the API signature. Likely need to restructure the backend to wrap updates in a discriminated envelope { "wallet-update": ... } and dispatch off the envelope key, or check if a newer library version supports mark dispatch.
Logging desk/app/hodl.hoon %+ verb & left enabled with :: TODO: disable before production Disable or remove before merging to master
Dead code desk/app/hodl.hoon Commented-out on-watch subscription scoping code Remove before merging
Whitespace desk/app/hodl.hoon Trailing whitespace on [%updates ~] \this ` line Clean up
Typo desk/app/hodl.hoon :: TODO do these action need to be namesspaced? Fix typo ("namesspaced")
Duplicate import ui/src/views/Wallets/Wallets.tsx useWallets and useWalletsState imported on separate lines from the same module Merge into one import
No error handler ui/src/state/bootstrap.ts api.subscribe has no err callback Add an error handler for subscription failures
UX ui/src/views/Wallets/Wallets.tsx 💣 emoji delete button Use text ("delete") or a standard icon for consistency with the rest of the UI

Detailed Findings

1. Subscription mark dispatch (likely broken)

bootstrap.ts:

event: (data: any, mark: string) => {
  switch (mark) {
    case 'wallet-update': ...
    case 'account-update': ...
    case 'transaction-update': ...
    default:
      console.warn('Unknown subscription mark:', mark);
  }
}

The existing transactions.tsx (pre-PR) used event: (update: TransactionUpdate) => void with a single argument. @urbit/http-api's subscribe passes only data to the event callback — not mark. If mark is always undefined, every event falls to default and all subscription updates (add/edit/del for all three models) are silently dropped.

The test plan checks "Subscription facts emitted without errors on all poke operations" but likely only verified no Hoon-side errors, not that the frontend received and applied the updates. Recommend testing that a poke actually updates the UI without a page refresh.

One fix: restructure backend updates to wrap in a keyed envelope — e.g. { "wallet-update": { "add": {...} } } — and dispatch off the top-level key in a single event handler without relying on mark.

2. account-id not validated or required in TransactionsForm

TransactionsForm.tsx has no { required: true } on register('account-id') — a transaction can be submitted with account-id = ''. The backend accepts this (migration backfills with ''), but new transactions with no account association may cause issues downstream. Consider either requiring it or making the omission explicit.

3. past namespace in sur/transaction.hoon appears unused

/-  zer=transaction-0
|%
++  past
  |%
  ++  zero  zer
  --

The agent defines txn-0/txns-0 inline and doesn't reference past from the transaction sur. This is either dead code or for future use — a comment explaining intent would help.


Questions

  1. Was subscription end-to-end tested (does an add poke from the UI actually update the list without refresh)? The mark dispatch concern above suggests it may not be working.
  2. Is account-id intentionally optional on new transactions, or should the form require it?
  3. Is past.zero in sur/transaction.hoon intended for external consumers, or can it be removed?

Positive Observations

  • State migration using |^ with explicit state-0-to-1 arm is clean — good Hoon idiom.
  • Inlining txn-0/txns-0 in the agent to work around the import chain issue is pragmatic.
  • Replacing four separate localStorage/Context stores with a single bootstrapStores() is a real improvement.
  • Per-mark poke handlers in the helper core are well-structured and consistent.
  • sur/transaction-0.hoon preservation strategy for migration is the right approach.
  • All parseFloat callsites in transaction views correctly updated for the string type change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant