feat: add wallet-scoped activity storage#103
Conversation
|
Seems ok architecturally; Though if noticed that it still looks up for pre-activity metadata by address or payment id only, then writes the tags into the wallet of the activity being inserted. I reproduced this with Codex using the Python binding: metadata added for an address was attached to a Could we add |
|
The top-level README still shows the old activity signatures without |
ovitrif
left a comment
There was a problem hiding this comment.
Re-reviewed the latest push. The original PreActivityMetadata wallet-scope issue and the root README activity API docs issue are mostly addressed. I found a few follow-up points:
src/modules/activity/implementation.rs:897
Could we make the bulk upsert path apply pending pre-activity metadata too? insert_activity transfers and deletes metadata, but upsert_onchain_activities, upsert_lightning_activities, and upsert_activities just write rows. I reproduced this through the Python binding: metadata tags stay pending and the inserted activity has no tags after upsert_onchain_activities. Android watcher sync is likely to use this bulk path, so hardware activities would miss the same tags and metadata support this PR is adding.
src/modules/activity/implementation.rs:2546
Could we include is_receive = 1 in the address lookup used for received onchain transfers? Right now a metadata row for the same address with is_receive = false is selected for a received activity, then cleanup leaves it behind because it only deletes is_receive = 1. I reproduced this through the Python binding: the received activity got the sent-only tag and the pending row remained.
src/lib.rs:1685
Should is_address_used take a wallet_id now too? It still counts any onchain activity matching address across all wallets. I reproduced a hardware-only activity making is_address_used(address) return true even though the default wallet had no matching activity. That can make the Bitkit wallet treat a hardware-wallet address as already used.
src/modules/activity/README.md:286
Could we re-check these examples against the generated bindings? The Kotlin enum payloads are under v1, so foundActivity.txId and foundActivity.preimage will not compile, and there is an extra } after the catch block. The Python sample also omits required OnchainActivity fields like contact, created_at, updated_at, and seen_at, and some get_activities calls omit nullable parameters that the generated Python API still requires.
Local checks:
cargo fmt --check: passedcargo test modules::activity: passed, 159 tests
Leaving this as a neutral review.
|
Thanks Ovi, pushed a follow-up in 8cde6b8 covering the review points:
Added regression coverage for the update/bulk metadata transfer and receive-address replacement cases. Latest local checks: |
ovitrif
left a comment
There was a problem hiding this comment.
Re-reviewed the latest push. The stale bulk-upsert, received-address lookup, and activity README example points are fixed in the Rust source, so I’m only leaving the two current follow-ups I could still verify.
Local checks:
cargo fmt --check: passedgit diff --check origin/master...HEAD: passedcargo test modules::activity: passed, 164 tests- Python binding smoke test: import works, but the committed native artifact still reproduces the old metadata behavior noted below
Leaving this as a neutral review.
ovitrif
left a comment
There was a problem hiding this comment.
LGTM, thanks for addressing the follow-ups.
Summary
wallet_idto activity, activity tag, and transaction detail data so multiple wallets can safely store overlapping activity IDs or transaction IDs.0.3.0.Closes #102.
Validation
cargo test modules::activitypassed: 155 tests./build.sh -r --minor allperformed the version bump; after the initial sandbox DNS failure,./build.sh allcompleted successfullygit diff --checkpassed