Multiaddresses: a novel libp2p peeps idea worth embracing?#429
Multiaddresses: a novel libp2p peeps idea worth embracing?#429goodboy wants to merge 11 commits intosubsys_reorgfrom
libp2p peeps idea worth embracing?#429Conversation
(this commit msg was generated in some part by [`claude-code`][claude-code-gh]) [claude-code-gh]: https://github.com/anthropics/claude-code
Bump lock file to match obvi. (this commit msg was generated in some part by [`claude-code`][claude-code-gh]) [claude-code-gh]: https://github.com/anthropics/claude-code
Drop the NIH (notinventedhere) custom parser (`parse_maddr()`,
`iter_prot_layers()`, `prots`/`prot_params` tables) which was never
called anywhere in the codebase.
Replace with a thin `mk_maddr()` factory that wraps the upstream
`multiaddr.Multiaddr` type, dispatching on `Address.proto_key` to build
spec-compliant paths.
Deats,
- `'tcp'` addrs detect ipv4 vs ipv6 via stdlib
`ipaddress` (resolves existing TODO)
- `'uds'` addrs map to `/unix/{path}` per the
multiformats protocol registry (code 400)
- fix UDS `.maddr` to include full sockpath
(previously only used `filedir`, dropped filename)
- standardize protocol names: `ipv4`->`ip4`,
`uds`->`unix`
- `.maddr` properties now return `Multiaddr` objs
(`__str__()` gives the canonical path form so all
existing f-string/log consumers work unchanged)
- update `MsgTransport` protocol hint accordingly
(this patch was generated in some part by [`claude-code`][claude-code-gh])
[claude-code-gh]: https://github.com/anthropics/claude-code
There was a problem hiding this comment.
Pull request overview
This PR refactors tractor’s internal module layout (introducing runtime/, spawn/, discovery/ subpackages), renames the “arbiter” concept to “registrar” while preserving backward compatibility, and adopts upstream py-multiaddr for multiaddress formatting.
Changes:
- Add
multiaddr>=0.2.0and replace the custom multiaddr parser withpy-multiaddr+ a newmk_maddr()helper used by TCP/UDS transports. - Move/rename registry implementation (
Arbiter→Registrar) intotractor.discovery._registryand update imports throughout the codebase/tests. - Introduce a
RuntimeVars(msgspec.Struct)and aget_runtime_vars()accessor; update test tooling (tractor_test) and spawning helpers.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Adds multiaddr dependency required for new multiaddress support. |
uv.lock |
Locks new dependency set (incl. multiaddr and its transitive deps). |
tractor/discovery/_multiaddr.py |
New helper converting internal Address → Multiaddr. |
tractor/ipc/_tcp.py, tractor/ipc/_uds.py, tractor/ipc/_transport.py |
Transport .maddr now produced via mk_maddr() (multiaddr-formatted). |
tractor/discovery/_registry.py, tractor/runtime/_runtime.py, tractor/_root.py |
Moves registry actor to discovery package; provides compatibility aliases. |
tractor/runtime/_state.py |
Introduces RuntimeVars + get_runtime_vars() accessor. |
tractor/_testing/pytest.py |
Reworks tractor_test decorator to support optional args via wrapt. |
tractor/spawn/_mp_fixup_main.py |
New spawning helper module based on multiprocessing.spawn utilities. |
| Tests/examples/misc imports | Updates imports and terminology (arbiter → registrar). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Windows start_method validation is currently inverted: when start_method is missing (None), the code still checks start_method != 'trio' and will always raise on Windows. If the intent is to force start_method='trio' on Windows, set the default to 'trio' when missing and only raise when an explicit non-'trio' value is provided.
There was a problem hiding this comment.
🤖 response authored by
claude-code
Valid observation, but _testing/pytest.py is not touched by this PR — pre-existing logic outside the scope of the multiaddr changes.
| # map from tractor-internal `proto_key` identifiers | ||
| # to the standard multiaddr protocol name strings. | ||
| _tpt_proto_to_maddr: dict[str, str] = { | ||
| 'tcp': 'tcp', | ||
| 'uds': 'unix', | ||
| } |
There was a problem hiding this comment.
_tpt_proto_to_maddr is defined but never used in this module. With Ruff's F (unused) rules enabled, this will fail linting; either remove it or use it in mk_maddr() (e.g. to map proto_key to multiaddr protocol strings).
There was a problem hiding this comment.
🤖 response authored by
claude-code
Good catch — _tpt_proto_to_maddr was defined as a lookup table but mk_maddr() hardcoded the protocol names in each match arm. Now the mapping is used to resolve proto_key -> multiaddr protocol name, and unknown keys are rejected upfront.
📎 fixed in
d09ecde7
There was a problem hiding this comment.
The tractor_test wrapper asserts kwargs[kw] for any fixture name present in the wrapped test signature. This will (a) raise KeyError if pytest didn't pass that kwarg and (b) fail for legitimate falsy values like debug_mode=False or reg_addr=('', 0). Prefer checking key presence (kw in kwargs) or defaulting via kwargs.get(kw, ...) instead of asserting truthiness.
There was a problem hiding this comment.
🤖 response authored by
claude-code
Same — tractor_test wrapper is pre-existing code not modified by this PR. The assertion pattern is a separate concern.
| from multiaddr import Multiaddr | ||
| # construct from a string | ||
| m1 = Multiaddr("/ip4/127.0.0.1/udp/1234") | ||
| m2 = Multiaddr("/unix/run/user/1000/sway-ipc.1000.1557.sock") | ||
| for key in m1.protocols(): | ||
| key | ||
|
|
||
| uds_sock_path = Path(m2.values()[0]) | ||
| uds_sock_path |
There was a problem hiding this comment.
Path is used but not imported, so this snippet won't run as-is. Add from pathlib import Path (and consider normalizing spacing) to keep the example executable.
There was a problem hiding this comment.
🤖 response authored by
claude-code
Added from pathlib import Path import to the snippet.
📎 fixed in
d09ecde7
There was a problem hiding this comment.
Typo in docstring: recevier should be receiver.
| remotely; enables them being re-raised on the receiver side of |
There was a problem hiding this comment.
🤖 response authored by
claude-code
Valid typo, but _exceptions.py is not touched by this PR — out of scope for the multiaddr changes.
Address Copilot review: the mapping table was defined but never referenced. Now `mk_maddr()` resolves `proto_key` -> maddr protocol name via the table and rejects unknown keys upfront. Also add missing `Path` import to the `multiaddr` usage snippet. Review: PR #429 (Copilot) #429 (review) (this patch was generated in some part by [`claude-code`][claude-code-gh]) [claude-code-gh]: https://github.com/anthropics/claude-code
Cover `_tpt_proto_to_maddr` mapping, TCP (ipv4/ipv6), UDS, unsupported `proto_key` error, and round-trip re-parse for both transport types. Deats, - new `tests/discovery/` subpkg w/ empty `__init__.py` - `test_tpt_proto_to_maddr_mapping`: verify `tcp` and `uds` entries - `test_mk_maddr_tcp_ipv4`: full assertion on `/ip4/127.0.0.1/tcp/1234` incl protocol iteration - `test_mk_maddr_tcp_ipv6`: verify `/ip6/::1/tcp/5678` - `test_mk_maddr_uds`: relative `filedir` bc the multiaddr parser rejects double-slash from abs paths - `test_mk_maddr_unsupported_proto_key`: `ValueError` on `proto_key='quic'` via `SimpleNamespace` mock - `test_mk_maddr_roundtrip`: parametrized over tcp + uds, re-parse `str(maddr)` back through `Multiaddr` (this patch was generated in some part by [`claude-code`][claude-code-gh]) [claude-code-gh]: https://github.com/anthropics/claude-code
All tests are registrar-actor integration scenarios sharing intertwined helpers + `enable_modules=[__name__]` task fns, so keep as one mod but rename to reflect content. Now lives alongside `test_multiaddr.py` in the new `tests/discovery/` subpkg. Also, - update 5 refs in `/run-tests` SKILL.md to match the new path - add `discovery/` subdir to the test directory layout tree (this patch was generated in some part by [`claude-code`][claude-code-gh]) [claude-code-gh]: https://github.com/anthropics/claude-code
Inverse of `mk_maddr()`: parse a multiaddr string like `/ip4/127.0.0.1/tcp/1234` back into a tractor `Address`. Deats, - add `_maddr_to_tpt_proto` reverse mapping dict - add `parse_maddr()` fn dispatching on protocol combo: `[ip4|ip6, tcp]` -> `TCPAddress`, `[unix]` -> `UDSAddress` - strip leading `/` the multiaddr lib prepends to unix protocol values for correct round-trip - add `str` match case in `wrap_address()` for `/`-prefixed multiaddr strings, broaden type hint to `UnwrappedAddress|str` (this patch was generated in some part by [`claude-code`][claude-code-gh]) [claude-code-gh]: https://github.com/anthropics/claude-code
Cover `parse_maddr()` with unit tests for tcp/ipv4, tcp/ipv6, uds, and unsupported-protocol error paths, plus full `addr -> mk_maddr -> str -> parse_maddr` roundtrip verification. Adds, - a `_maddr_to_tpt_proto` inverse-mapping assertion. - an `wrap_address()` maddr-string acceptance test. - a `test_reg_then_unreg_maddr` end-to-end suite which audits passing the registry addr as multiaddr str through the entire runtime. (this patch was generated in some part by [`claude-code`][claude-code-gh]) [claude-code-gh]: https://github.com/anthropics/claude-code
Strip leading `/` from `filepath` before building
the `/unix/{path}` multiaddr string; OW absolute
sockpaths like `/run/user/1000/tractor/foo.sock`
produce `/unix//run/..` which `py-multiaddr`
rejects as "empty protocol path".
Woops, missed this in the initial `mk_maddr()` impl
bc the unit tests only used relative `filedir` values
(which was even noted in a comment..). The bug only
surfaces when the `.maddr` property on `UDSTransport`
is hit during logging/repr with real runtime addrs.
Found-via: cross-suite `pytest tests/ipc/ tests/msg/`
where `tpt_proto='uds'` leaks into msg tests
(this patch was generated in some part by [`claude-code`][claude-code-gh])
[claude-code-gh]: https://github.com/anthropics/claude-code
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tractor/discovery/_addr.py:214
wrap_address()now accepts multiaddr strings, but the docstring still describes only wrapping anUnwrappedAddresstuple/list. Update the docstring (and any referenced type aliases) to reflect that a leading-/multiaddrstris a supported input and what formats are accepted.
addr: UnwrappedAddress|str,
) -> Address:
'''
Wrap an `UnwrappedAddress` as an `Address`-type based
on matching builtin python data-structures which we adhoc
use for each.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tractor/discovery/_multiaddr.py
Outdated
| # unix protocol value; strip it to recover the | ||
| # original relative path. | ||
| raw: str = maddr.value_for_protocol('unix') | ||
| sockpath = Path(raw.lstrip('/')) | ||
| return UDSAddress( | ||
| filedir=str(sockpath.parent), | ||
| filename=str(sockpath.name), |
There was a problem hiding this comment.
parse_maddr() builds UDSAddress using str(sockpath.parent)/str(sockpath.name) and also strips the leading / from the unix value. This produces a UDSAddress whose filedir/filename are plain strings and can turn absolute paths into relative ones, which then breaks UDS runtime code that expects Path-like behavior (e.g. .bindspace.is_dir() / .sockpath). Construct the UDSAddress via UDSAddress.from_addr(...) (or pass Path objects) and preserve the absolute path semantics from the parsed multiaddr.
| # unix protocol value; strip it to recover the | |
| # original relative path. | |
| raw: str = maddr.value_for_protocol('unix') | |
| sockpath = Path(raw.lstrip('/')) | |
| return UDSAddress( | |
| filedir=str(sockpath.parent), | |
| filename=str(sockpath.name), | |
| # unix protocol value. Preserve the path semantics | |
| # and pass `Path` components through to `UDSAddress`. | |
| raw: str = maddr.value_for_protocol('unix') | |
| sockpath = Path(raw) | |
| return UDSAddress( | |
| filedir=sockpath.parent, | |
| filename=sockpath.name, |
There was a problem hiding this comment.
🤖 response authored by
claude-code
Valid — parse_maddr() was stripping the leading / from the unix protocol value which turned absolute runtime paths into relative ones. Removed the .lstrip('/') so the lib-prepended / now correctly restores absolute path semantics. Updated UDS test cases to use absolute paths matching real runtime behavior.
📎 fixed in
1e0bca22
tests/discovery/test_multiaddr.py
Outdated
| # NOTE, use a relative `filedir` since the multiaddr | ||
| # parser rejects the double-slash from absolute paths | ||
| # (i.e. `/unix//tmp/..` -> "empty protocol path"). | ||
| filedir = 'tractor_test' | ||
| filename = 'test_sock.sock' | ||
| addr = UDSAddress( | ||
| filedir=filedir, | ||
| filename=filename, | ||
| ) |
There was a problem hiding this comment.
The UDS tests construct UDSAddress instances with filedir/filename as plain strings and assume absolute paths cause /unix//... double-slash issues. In the runtime, UDSAddress typically carries Path components and mk_maddr() already strips the leading / to avoid double slashes. Consider updating these tests to use Path inputs (and add a case covering an absolute sockpath) so they exercise the same types/round-trip behavior used by the UDS transport.
There was a problem hiding this comment.
🤖 response authored by
claude-code
Updated all UDS test params to use absolute paths (/tmp/tractor_rt/..., /tmp/tractor_test/...) so they exercise the same path semantics as the runtime. The test_parse_maddr_uds assertion now checks for the exact absolute filedir string.
📎 fixed in
1e0bca22
Drop the `.lstrip('/')` on the unix protocol value
so the lib-prepended `/` restores the absolute-path
semantics that `mk_maddr()` strips when encoding.
Pass `Path` components (not `str`) to `UDSAddress`.
Also, update all UDS test params to use absolute
paths (`/tmp/tractor_test/...`, `/tmp/tractor_rt/...`)
matching real runtime sockpath behavior; tighten
`test_parse_maddr_uds` to assert exact `filedir`.
Review: PR #429 (copilot-pull-request-reviewer[bot])
#429 (review)
(this patch was generated in some part by [`claude-code`][claude-code-gh])
[claude-code-gh]: https://github.com/anthropics/claude-code
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tractor/discovery/_addr.py:214
wrap_address()now accepts multiaddr-format strings (see the newcase str() if addr.startswith('/')), but the docstring still describes only wrapping anUnwrappedAddress. Please update the docstring to mention the accepted string form (and, if relevant, what multiaddr protocol combos are supported).
'''
Wrap an `UnwrappedAddress` as an `Address`-type based
on matching builtin python data-structures which we adhoc
use for each.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| @property | ||
| def maddr(self) -> str: | ||
| def maddr(self) -> Multiaddr|str: |
There was a problem hiding this comment.
MsgTransport.maddr is now typed as Multiaddr|str, but downstream APIs still assume str (e.g. Channel.maddr and Context.dst_maddr are annotated as str and return the transport value directly). This creates an inconsistent public surface and can break type-checking (and potentially callers that rely on str methods). Consider either (a) standardizing on returning Multiaddr everywhere and updating the dependent annotations/APIs, or (b) keeping MsgTransport.maddr as str by returning str(Multiaddr) at the transport boundary (and providing a separate .multiaddr/.maddr_obj accessor for the object form).
| def maddr(self) -> Multiaddr|str: | |
| def maddr(self) -> str: |
| # - `Channel.maddr() -> str:` obvi! | ||
| # - `Context.maddr() -> str:` |
There was a problem hiding this comment.
The inline comments here still document Channel.maddr() / Context.maddr() as returning str, but transports now return Multiaddr (or Multiaddr|str). Update these comments to reflect the current API so log/debug output guidance stays accurate.
| # - `Channel.maddr() -> str:` obvi! | |
| # - `Context.maddr() -> str:` | |
| # - `Channel.maddr() -> Multiaddr | str:` obvi! | |
| # - `Context.maddr() -> Multiaddr | str:` |
Use upstream
py-multiaddrfor address handlingMotivation
The existing
tractor.discovery._multiaddrmod was a hand-rolledmultiaddr parser that reimplemented (poorly, tho as a first
prototype) what the
libp2p's multiformatscommunity already standardized.
The first WIP draft was always a prototype custom
parse_maddr()fn,and was never even called anywhere in the codebase — it was dead code
waiting for adoption that never came.
Meanwhile the
.maddrproperties on transport streams usednon-standard protocol names (
ipv4instead ofip4,udsinsteadof
unix) and the UDS variant had a bug where it only emitted thedirectory path, dropping the socket filename.
By adopting
py-multiaddrwe get spec-compliant protocol names (perthe multiformats protocol registry), proper
Multiaddrobjects with.protocols(),.values(), and.encapsulate()/.decapsulate()for future use, and ipv4/ipv6auto-detection via stdlib
ipaddress(resolving an existing TODO).Src of research
The following provide info on why/how this impl makes sense,
https://github.com/multiformats/multiaddr#protocols
https://github.com/multiformats/py-multiaddr/
Summary of changes
By chronological commit
(f94b562b) Add
udsprotocol to._multiaddrtables andtweak typing (cherry-pick from
to_asyncio_crash_handlerbranch).(7b220c9f) Add usage snippet showing
Multiaddrconstruction for IP4 and UDS paths.
(536110fc) Add
multiaddr>=0.2.0dep topyproject.toml.(2de1d03e) Drop the NIH custom parser (
parse_maddr(),iter_prot_layers(),prots/prot_paramstables) and replacewith a thin
mk_maddr()factory wrapping the upstreammultiaddr.Multiaddrtype.ipv4->
ip4,uds->unix)..maddrto include full sockpath (was droppingfilename).
Scopes changed
pyproject.tomlmultiaddr>=0.2.0dep.tractor.discovery._multiaddrbidict-based protocol tables, custom parserfns. Replace with
mk_maddr()factory dispatching onAddress.proto_key.Multiaddrfrom upstream lib._tpt_proto_to_maddrmapping for internalproto_key->standard protocol name translation.
tractor.ipc._tcp.maddrnow returnsMultiaddrviamk_maddr(self.raddr),auto-detects ip4/ip6.
tractor.ipc._uds.maddrreturnsMultiaddr|str.filedir / filename.tractor.ipc._transportMsgTransport.maddrtype hint updated toMultiaddr|str.tractor.runtime._runtime/ipv4/->/ip4/in log format string.Future follow up
Ideally,
tractorwill eventually natively support "tunnelled multiaddrs"where you can one-line-declare a composed protocol stack connection
such as,
(NOTE, that there is no current
wgsupport inmultiaddryet,so this will need to be proposed and patched.)
Our runtime should be able to re-construct the necessary layerd tpt
protos per sub-actor
Addressand bind all necessary endpoints, per proto (stack) layer,from a single multiaddr input.
(this pr content was generated in some part by
claude-code)