tapgarden: surface non-critical custodian errors to subscribers#2070
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the observability of the asset receive process within the tapgarden custodian. By surfacing non-critical errors to subscribers, it enables external systems to react to failures during transaction inspection, mailbox message handling, and proof mapping. This change ensures that subscribers receive timely feedback on the status of their asset receives, improving overall system transparency and debugging capabilities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request enhances the Custodian by publishing error events to subscribers during asset reception failures in wallet transaction inspection, mailbox message handling, and proof mapping. It also refines proof mapping to capture specific event context and adds unit tests for these mechanisms. A review comment suggests optimizing event lookups in mapProofToEvent by using direct map access instead of iteration.
Coverage Report for CI Build 24594654080Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.1%) to 34.385%Details
Uncovered Changes
Coverage Regressions1456 previously-covered lines in 22 files lost coverage.
Coverage Stats
💛 - Coveralls |
jtobin
left a comment
There was a problem hiding this comment.
Just a few changes required IMO:
- I wrote a couple of comments re: a nil pointer deref problem and not explicitly handling decryption errors in handleMailboxMessages, please check those out.
- The fix-up commits should be squashed into the commits that introduce the changes, or just a single fix-up commit would also be fine IMO. Please make sure they obey the
change target: change summarystyle of other commits, too.
d857e4f to
7add191
Compare
|
Hey Jared, thanks for the review! Addressed both points:
Also squashed everything into a single commit. |
jtobin
left a comment
There was a problem hiding this comment.
One problem introduced in the last changeset, otherwise looks clean.
Publish AssetReceiveErrorEvent in error paths that previously only logged, so RPC clients subscribed via SubscribeReceiveEvents can be notified of failures. Changes: - inspectWalletTx: publish error events when NewTransferFromWalletTx or GetOrCreateEvent fails - mapProofToEvent: refactor to capture matching event, publish errors when FetchProof or assertProofInLocalArchive fails - handleMailboxMessages: publish error events for script key, block fetch, and event creation failures For errors occurring before full context is available, use defaults: confHeight=0, status=StatusTransactionDetected. Add TestCustodianEventSubscriber to verify subscriber mechanism and TestAssetReceiveErrorEvent to verify error event structure. Fixes lightninglabs#1942 Signed-off-by: kaldun-tech <tsmereka@protonmail.com>
7add191 to
3934ef5
Compare
|
|
||
| // Save these before GetOrCreateEvent, which may | ||
| // return nil on error. | ||
| tapAddr := event.Addr.Tap |
There was a problem hiding this comment.
I pass the stored *tapAddr and op (Outpoint from line 717) into the ErrorEvents
b3f94c4
Fixes #1942.
Adds
NewAssetReceiveErrorEventto publish error events through the existing subscriber mechanism. Error paths ininspectWalletTx,handleMailboxMessages, andmapProofToEventnow notify subscribers before returning/continuing on errors.This enables subscribers (e.g., RPC clients watching for receives) to be notified about errors during asset receive processing, rather than only learning about successful completions or silent failures logged to the daemon.
Unit tests added for both the subscriber event flow and error event structure.