refactor: clean up dependencies and remove unused bimodal PDP-less operation#6
Merged
Conversation
Adds pdpfake — in-memory types.PieceAPI and commp.Calculator fakes plus an fx module that exposes them as pdp.PDP — and switches the three existing fxtest suites to wire them in alongside app.UCANModule. Today the tests are the only consumers of the non-PDP code path: every fxtest setup omits app.PDPModule, leaving pdp.PDP nil in the handler graph, and tests seed bytes via svc.Blobs().Store().Put. Subsequent commits will delete that path entirely; routing the tests through PDP first lets the deletion remain a pure-removal commit. No production code changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Deletes the bimodal seams: the six `if s.PDP() == nil` branches in handlers (allocate, accept, replica/transfer, pdp_info), the `if params.API != nil` adapter switch in retrieval wiring, and the `optional:"true"` tags on PDP fields in the storage and replicator fx providers. With the non-PDP path gone, the byte-storage surface on the blobs service is no longer reachable: removes Store, Presigner and Access from the blobs.Blobs interface; deletes pkg/service/blobs/server.go (the GET/PUT /blob/:blob handler), pkg/access and pkg/presigner packages, and pkg/fx/presigner. blobs.Blobs now exposes only Allocations and Acceptances. Also retires the deprecated blobstore.PDPStore alias. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the fat view interfaces (AllocateService, AcceptService, TransferService, BlobAllocateService, BlobAcceptService, ReplicaAllocateService, PDPInfoService, AccessGrantService) with per-handler `*Deps` structs populated by fx, each declaring its own narrow consumer-defined interfaces next to the handler: - AllocationStore, PieceAllocator alongside Allocate - AcceptanceStore, PieceReader alongside Accept - PieceResolver alongside pdp/info The Deps structs use fx.In, so wiring stays declarative — pkg/fx/storage gains a handful of identity providers that bind concrete production types (allocationstore.AllocationStore, types.PieceAPI) to the narrow handler-side interfaces, and a NewUploadConnection provider that extracts the upload client.Connection from AppConfig as a top-level bean. The replicator's adapter struct goes away; the Service now constructs a TransferDeps value once and reuses it for the queue handler and the failure receipt. transferBlobFromSource builds an AcceptDeps subset from the TransferDeps to call into blobhandler.Accept. Tests stop populating `var svc storage.Service` and instead populate the narrow types they actually assert against. The umbrella interfaces still exist as composition (storageServiceWrapper) but no longer satisfy any view interface — commit 4 deletes them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removes the umbrellas that the per-handler Deps refactor made obsolete: - storage.Service (and the storageServiceWrapper that satisfied it) - pdp.PDP (and TODO_PDP_Impl, ProvideTODOPDPImplInterface) - blobs.Blobs (and the entire pkg/service/blobs + pkg/fx/blobs) The replicator constructor now takes the narrow concrete deps (types.PieceAPI, commp.Calculator, acceptancestore.AcceptanceStore) directly instead of the pdp.PDP and blobs.Blobs umbrellas. The PDP fx module provides types.PieceReaderAPI via fx.As so retrieval's non-optional API field resolves without going through TODO_PDP_Impl. pdpfake's fx module no longer manufactures a pdp.PDP wrapper — it provides types.PieceAPI / types.PieceReaderAPI / commp.Calculator directly, mirroring the production wiring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds table-driven unit tests against Allocate and Accept that operate on
the per-handler Deps structs directly, with in-memory implementations
(allocationstore + acceptancestore backed by go-datastore MapDatastore,
plus pdpfake). The full suite for pkg/service/storage/handlers/blob
runs in ~17ms versus ~7s for the fxtest-based integration tests in
pkg/service/storage, with coverage at 72.6%.
Allocate coverage focuses on the decision tree:
- no prior allocation, blob not received → size = requested, address set
- allocation exists, blob not received → size = 0, address set
- allocation exists, blob received → size = 0, address nil
- allocation in another space, blob received → size = requested, address nil
- unsupported hash → error
Accept coverage:
- piece absent → "piece not found" error
- piece held → returns Claim + PDP invocation, persists acceptance with
pdp/accept promise, persists location claim, calls publisher exactly once
Transfer unit tests are intentionally deferred — the handler is mostly
HTTP/UCAN glue best covered by the existing fxtest integration tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Applies the same Deps-struct pattern to retrieval that we used for
storage. The shape was identical: a fat retrieval.Service umbrella
(ID + Allocations + Blobs() blobstore.BlobGetter), a RetrievalService
wrapper, and per-handler view interfaces (BlobRetrievalService,
SpaceContentRetrievalService) that handlers reached through to call
.Blobs().Get(). With those gone, the only thing keeping
blobstore.BlobGetter alive was a BlobGetterAdapter wrapping
types.PieceReaderAPI to satisfy it.
- spacecontent.Retrieve now takes types.PieceReaderAPI directly and
calls pieces.Read returning *types.PieceReader. Range errors still
propagate as blobstore.RangeNotSatisfiableError through the
production piece.StoreReader chain, so the test assertions remain
valid.
- WithBlobRetrieveMethod and WithSpaceContentRetrieveMethod take new
BlobRetrieveDeps / SpaceContentRetrieveDeps structs (fx.In).
- pkg/service/retrieval/{interface,service}.go deleted (umbrella + wrapper).
- pkg/pdp/store/adapter deleted (the BlobGetterAdapter was its only purpose).
- pkg/fx/retrieval/provider.go deleted — the retrieval module had no
remaining providers; pkg/fx/retrieval/ucan.Module still wires the
UCAN handlers.
- blobstore.BlobGetter removed; Get is folded directly into Blobstore.
- fx.As(new(blobstore.BlobGetter)) annotations dropped from the
memory/s3/filesystem store providers.
- Retrieval tests construct piece.NewStoreReader(blobstore...) to
produce a PieceReaderAPI from the existing in-memory blobstore —
using the production read path keeps RangeNotSatisfiableError
assertions accurate.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removes the last bag-style interfaces and a couple of dead scaffolds
that were leftover after the storage/retrieval refactor:
- claims.Claims (2-getter umbrella over ClaimStore + Publisher). Handlers
that used to chain `deps.Claims.Store().Put(...)` and
`deps.Claims.Publisher().Publish(...)` now take the two narrow deps
directly in their Deps structs.
- The whole pkg/service/claims/{interface.go,service.go} pair goes
with it (only server.go survives — the /claim/:claim route registrar).
- publisher.Publisher.Store() — the interface method had zero callers
outside a nil-returning test stub. The PublisherStore is already a
top-level fx bean. Removed from the interface, the impl method, and
the corresponding field on PublisherService.
- pkg/service/storage/server.go `Server` struct and
pkg/service/retrieval/server.go `Server` struct were never
instantiated. Only their NewHandler peers are used by fx.
- pkg/store/claimstore deleted — the package was a single file
aliasing delegationstore.DelegationStore with an author-acknowledged
"// TODO a glorified type alias, remove this" comment. All callers
switched to delegationstore.DelegationStore directly.
replicator.New's constructor and the matching fx Params struct now take
ClaimStore + Publisher fields instead of the umbrella.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Moves the parallel pkg/fx/ tree's service-mirror modules into the
packages they describe. Each impl package now owns its own fx wiring as
fx.go, eliminating the maintenance tax of keeping two parallel trees in
sync (which already failed once — pkg/fx/blobs lingered after the impl
side was deleted, and we caught pkg/fx/retrieval in the BlobGetter
removal). The fx framework is no longer hidden behind a parallel
directory; the colocation makes deletions and renames atomic.
Moves:
- pkg/fx/claims → pkg/service/claims/fx.go
- pkg/fx/replicator → pkg/service/replicator/fx.go
- pkg/fx/publisher → pkg/service/publisher/fx.go
- pkg/fx/proofs → pkg/service/proofs/fx.go
- pkg/fx/principalresolver → pkg/principalresolver/fx.go
- pkg/fx/wallet → pkg/wallet/fx.go
- pkg/fx/storage/ → pkg/service/storage/fx.go
- pkg/fx/storage/ucan/{,handlers} → merged into pkg/service/storage/ucan/fx.go
- pkg/fx/retrieval/ucan/{,handlers} → merged into pkg/service/retrieval/ucan/fx.go
Stays in pkg/fx/ (legitimately framework-only — no impl-package home):
app, echo, root, database, store, pdp, scheduler, identity,
claimvalidation.
The storage/ucan and retrieval/ucan merges fold the former "outer"
provider (Handler + Module) and the "inner" handlers Module (per-ability
fx.Annotate calls) into a single Module per package. To avoid a
parent→child cycle in the storage tree (where pkg/service/storage/fx.go
would need pkg/service/storage/ucan.PieceResolver while
pkg/service/storage/ucan/fx.go needs storage.NewHandler), the echo
handler that wraps the UCAN server moved into the ucan subpackage as an
unexported newEchoHandler, and pkg/service/{storage,retrieval}/server.go
were deleted.
No behavior change. The fx graph composition is identical; the only
visible difference is import paths in pkg/fx/app/{ucan,common,pdp}.go.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ructor
Treats the AppConfig-as-context-bag pattern as the same smell that bag
interfaces were. Services no longer take app.AppConfig and reach inside
for the one or two fields they need — instead, CommonModules supplies
each meaningful sub-config struct via fx.Supply, and consumers request
just what they actually use.
CommonModules additions (pkg/fx/app/common.go):
fx.Supply(cfg.UCANService.Services) // ExternalServicesConfig
fx.Supply(cfg.UCANService.Services.Upload) // UploadServiceConfig
fx.Supply(cfg.UCANService.Services.Indexer) // IndexingServiceConfig
fx.Supply(cfg.UCANService.Services.Publisher) // PublisherServiceConfig
fx.Supply(cfg.UCANService.Services.EgressTracker)
Consumer narrowing:
- identity.ProvideIdentity AppConfig → IdentityConfig
- principalresolver.NewFx AppConfig → UCANServiceConfig
- publisher.NewFx AppConfig → PublisherServiceConfig + IndexingServiceConfig
- retrieval/ucan.NewServerHandler Params.Config → Upload UploadServiceConfig
- storage handler Deps types (AccessGrantDeps,
TransferDeps) replace `UploadConn client.Connection`
with `Upload app.UploadServiceConfig`; handler bodies
read `.Connection` from the struct.
The top-level `client.Connection` provider (storage.NewUploadConnection)
is deleted — it existed only to lift one config field into the fx graph
as a bare primitive. With the source struct supplied directly, the lift
isn't needed.
With that done, replicator's two-step constructor (positional New,
fx-wrapper NewFx) has no remaining justification. Its only callers were
inside the fx wrapper, and its config dep is now a single
UploadServiceConfig field. Collapsed into a single fx.In Params-based
constructor.
Publisher keeps its two-constructor shape: publisher.New(id, store,
addr, opts...) is the variadic-options API that publisher_test.go
exercises directly with custom options like WithLogLevel. publisher.NewFx
wraps it for fx callers — that wrapping is the real config-translation
layer and earns its keep.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cleanup pass after the audit. Two categories of cuts.
Dead nil checks. After the claims/publisher umbrella refactor, both
blob.Accept and replica.createLocationAssertion unconditionally set
their pdp/accept invocation field — the nil checks at three call sites
were unreachable:
- pkg/service/storage/ucan/blob_accept.go:49 (resp.PDP)
- pkg/service/storage/handlers/replica/transfer.go:222 (acceptResp.PDP)
- pkg/service/storage/handlers/replica/transfer.go:241 (pdpAcceptInv)
Dead provider. pkg/service/egresstracker/fx.go had a ProvideConsolidationStore
function that was commented out of fx.Provide (line 35: //ProvideConsolidationStore).
Nothing referenced it; consolidation stores come from pkg/fx/store/{memory,
filesystem,s3} providers instead. Deleted the function and its leveldb
imports.
Five remaining AppConfig consumers narrowed to their actual config slice:
- ProvideEthClient(cfg AppConfig) → ProvideEthClient(cfg PDPServiceConfig)
- ProvideLotusClient(cfg AppConfig) → ProvideLotusClient(cfg PDPServiceConfig)
- StartEchoServer(cfg AppConfig, ...) → StartEchoServer(cfg ServerConfig, ...)
- ProvideReceiptsClient(cfg AppConfig) → ProvideReceiptsClient(cfg EgressTrackerServiceConfig)
- NewEgressTrackerService(... cfg AppConfig) → ServerConfig + EgressTrackerServiceConfig
Only CommonModules still takes app.AppConfig — and that's correct, it's
the assembly point that explodes the whole thing into the supply tree.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI's go-check job flagged three files as not gofmt-ed and required the aws-sdk-go-v2 dependencies (no longer used after the s3 module cleanup in this branch) to be removed from go.mod via go mod tidy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alanshaw
approved these changes
May 18, 2026
|
|
||
| type Server struct { | ||
| claims claimstore.ClaimStore | ||
| claims delegationstore.DelegationStore |
Member
There was a problem hiding this comment.
BTW, claims in UCAN 1.0 end up being invocations. In the same way that receipts are attestations of execution, claims are just attestations of some other thing. The forcing function is that they need "arguments" in order to make claims so they cannot really be delegations.
Member
Author
There was a problem hiding this comment.
I imagine in later versions of this we'll swap this store with a more generic CBOR store.
|
|
||
| type Publisher interface { | ||
| // Store is the storage interface for published advertisements. | ||
| Store() store.PublisherStore |
Member
Author
There was a problem hiding this comment.
This method on the interface was never used.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three threads of dependency cleanup, working from the outermost smell inward:
if s.PDP() == nilto handle a no-PDP mode that's no longer a supported deployment. The two halves of every handler are now one.Depsstructs.storage.Service,pdp.PDP,blobs.Blobs,retrieval.Service,BlobGetter,claims.Claimswere bag-of-everything interfaces every handler took to use one or two methods. Each handler now declares its own narrowDepsstruct (the consumer-defined interface pattern) and fx wires exactly what it needs.pkg/fx/X/tree mirrored impl packages just to host their fx modules; modules now live next to the code they wire. Consumers take narrow sub-configs (ServerConfig,IndexingServiceConfig, …) instead of dragging inapp.AppConfig.Commits
bb9e039test: route tests through PDP path via in-memory fakes4347358refactor: drop non-PDP code pathsb212f17refactor: handlers take per-handler Deps structs7949520refactor: delete umbrella service interfaces889ebb0test: add direct unit tests for blob handlersddab410refactor: remove BlobGetter and the retrieval umbrellabec75b9fix: remove failing test that wrongly depended on old storacha infrab34054arefactor: drop claims/publisher umbrellas and dead Server scaffoldsf409d17refactor: inline fx modules into their impl packages6b0d6afrefactor: supply narrow config sub-structs; collapse replicator constructorab09476refactor: replace cliutil.ReadPrivateKeyFromPEM with library methodeb1964frefactor: trim dead nil checks and narrow remaining AppConfig consumers4796cb2chore: gofmt three files and tidy go.modWhat this enables
cfg.Xreach-through pattern (cfg.AppConfig.UCANService.Services.Indexer.Foo) collapses to single-segment access on a narrow type.Out of scope
Test plan
go build ./...go test -count=1 ./...passesgofmt -l .cleango mod tidyclean