|
| 1 | +# Onion Messaging Implementation Review |
| 2 | + |
| 3 | +## `onionmessage/hop.go` |
| 4 | + |
| 5 | +- [ ] **Refactor `processOnionMessage`**: |
| 6 | + The current implementation uses a chain of `fn.AndThen` which makes accessing intermediate results (like `processedPkt` and `payload`) in the final step awkward, necessitating `UnwrapOr(nil)`. |
| 7 | + **Suggestion**: Rewrite `processOnionMessage` using standard Go imperative error handling (`if err != nil`) to improve readability and avoid unsafe unwraps, or introduce a context struct passed through the pipeline. |
| 8 | + |
| 9 | +- [ ] **Magic Value in `processOnionMessagePacket`**: |
| 10 | + There is a TODO regarding the magic value `10` for `incomingCltv`. |
| 11 | + ```go |
| 12 | + // TODO(gijs): We should not use the magic value 10 here. |
| 13 | + ``` |
| 14 | + **Action**: Resolve this TODO, possibly by defining a constant or determining if a specific value is actually required by the sphinx router for this packet type. |
| 15 | + |
| 16 | +- [ ] **Documentation**: |
| 17 | + Add GoDoc comments for `ForwardAction`, `DeliverAction`, and `RoutingDecision` types. |
| 18 | + |
| 19 | +## `onionmessage/endpoint.go` |
| 20 | + |
| 21 | +- [ ] **Constructor Validation**: |
| 22 | + `NewOnionEndpoint` does not validate that `receptionist` or `router` are non-nil. |
| 23 | + **Suggestion**: Add checks or ensure these are mandatory. |
| 24 | + |
| 25 | +- [ ] **Error Handling in `forwardMessage`**: |
| 26 | + The error message `no actors found for nextNodeId` could be more descriptive (e.g., is the peer connected?). |
| 27 | + |
| 28 | +## `onionmessage/actor.go` |
| 29 | + |
| 30 | +- [ ] **Exported Fields**: |
| 31 | + `OMRequest` has an unexported field `msg`. Consider exporting it if external inspection is needed, or keep it unexported if it's strictly internal. |
| 32 | + |
| 33 | +- [ ] **Documentation**: |
| 34 | + Add comments explaining the role of `OMRequest` and `OMResponse` within the actor system adapter pattern. |
| 35 | + |
| 36 | +## `server.go` |
| 37 | + |
| 38 | +- [ ] **Replay Log TODO**: |
| 39 | + Resolve the TODO regarding `sphinx.NewMemoryReplayLog()`. |
| 40 | + ```go |
| 41 | + // TODO(gijs): remove the memory replay log once lightning-onion supports it. |
| 42 | + ``` |
| 43 | + **Action**: Check upstream `lightning-onion` status. |
| 44 | + |
| 45 | +## General |
| 46 | + |
| 47 | +- [ ] **Testing**: |
| 48 | + Ensure `itest/lnd_onion_message_forward_test.go` covers edge cases like: |
| 49 | + - Forwarding to a disconnected peer (Actor not found). |
| 50 | + - Malformed onion packets. |
| 51 | + - Context cancellation. |
0 commit comments