SDK #20 (draft): dincli/sdk foundation — loaders + manifest/runtime + state/serialize

[Santiagocetran]

Draft / WIP — progress on #20, not a merge-ready PR. Opened early per the request to keep feature-branch work visible. Base is feat/din-sdk (not develop).

What this contains

Incremental extraction of shared logic into dincli/sdk/ (issue #20), landed in reviewable slices:

1. Interface proposal (Developer/design/sdk-interface-proposal.md, v0.3) — SDK public interface + JSON output conventions; candidate seed for P3-DOC7. Direction confirmed by @umeradl; contract not yet frozen.
2. Package seed — dincli/sdk/{cid,errors}.py + a fresh-subprocess import-boundary test.
3. Loader tier — dincli/sdk/{config,log,web3}.py → {ipfs,contracts}.py → {manifest,runtime}.py, with the original modules (cli/utils.py, cli/log.py, services/{ipfs,runtime}.py, cli/contract_utils.py) reduced to backward-compat re-export shims.
4. State + serialization layer (this task — task_220726_7 Part A) — dincli/sdk/state.py and dincli/sdk/serialize.py (details below).

The dincli/sdk package now holds a dependency-clean foundation: cid, errors, config, log, web3, ipfs, contracts, manifest, runtime, state, serialize. An import-boundary test enforces the core rule — import dincli.sdk pulls in no typer, rich, or dincli.cli.* (CLI depends on SDK, never the reverse).

Latest slice — state.py + serialize.py (task_220726_7 Part A)

• dincli/sdk/state.py — GI-state converters (GIstateToDes/GIstateToStr/GIstatestrToIndex) and their backing data moved verbatim from cli/utils.py, which is now a re-export shim (call sites unchanged). Adds a GIState IntEnum (0-based, matching the Solidity enum ordinals) and two validate_* predicates lifted from DinContext's GI-state guards, raising the SDK's ValidationError. The two context.py guards now delegate the decision to these predicates while keeping identical console output + exit codes — no CLI behavior change.
• dincli/sdk/serialize.py — to_envelope() + a metadata-driven JSON encoder per proposal §3: the {status, data, error, meta} envelope (with schema_version/sdk_version) and the encoder rules — uint256_string → decimal string, omit, small-int passthrough, bytes/HexBytes → 0x hex, address → checksummed, enum → name, Decimal → string, None → null (except tagged omit).

Verification

• Unit suite (SDK + CLI unit tests): 144 passed (incl. 23 new state/serialize tests: one per encoder rule + both envelope paths, converter snapshot + shim-identity + predicate checks).
• Import-boundary test extended to assert state and serialize import clean of typer/rich/dincli.cli.* — green across all 11 SDK modules.
• dincli --help + full-package import smoke pass through the shims (behavior parity).
• Not run here (external toolchains, pre-existing, unaffected by this slice): the contract-integration suite (tests/dincli/*, needs the Hardhat/Foundry compile harness) and the Torch-dependent DP test (tests/test_cache_client_dp.py).

Scope vs #20 (explicitly partial)

• ✅ inventory · ✅ JSON conventions defined + implemented (serialize.py) · ✅ SDK tests · ✅ interface/back-compat documented
• ✅ shared-module extraction: config/log/web3/ipfs/contracts/manifest/runtime + state/serialize done
• ⬜ CLI-commands-render-JSON refactor (operations/ role layer) — not started (depends on wallet.py/session.py)
• ⏭️ Deferred to a dedicated next task — the wallet/session/tx keystone (wallet.py, session.py/DinSession, SignerProvider §2, tx.send() §5b). Split out deliberately: private-key/keystore/signing logic warrants its own focused review pass (flagged for @umeradl given the crypto sensitivity), not a shared slot with unrelated work. tx.send() also needs DinSession, which doesn't exist yet.

So this does not close #20; it's the extraction foundation the operations/ layer and the signing keystone build on.

Note on the diff

Base branch feat/din-sdk is behind develop, so the file diff includes the develop-sync commits (P3 design docs, upgradeable-proxy/ProxyAdmin work, task files). The SDK-specific commits are the docs(design) proposal, feat(sdk) seed + serializer, and the refactor(sdk) loader/state extraction commits.

[umeradl]

Code review — dincli/sdk foundation (loaders + manifest/runtime + state/serialize)

Reviewed at commit 6eac109 (feat/din-sdk, contributor Santiagocetran's fork), worktree /home/umerm/tempdir/DIN/DraftPRs/pr-31, diffed against origin/develop at 45cbfad. Scope: issue #20. Everything below was reproduced directly — actual diffs read line-by-line, actual test runs, actual git-blame comparisons against the pre-move code — not taken from the PR body.

git diff origin/develop...HEAD --stat: 28 files changed, 2682 insertions(+), 1046 deletions(-).

Files changed — PR #31 unique, at HEAD commit 6eac109 (name-status)

A	dincli/sdk/__init__.py
A	dincli/sdk/cid.py
A	dincli/sdk/ipfs.py
A	Developer/design/sdk-interface-proposal.md
M	dincli/cli/context.py
M	dincli/cli/contract_utils.py
A	dincli/sdk/contracts.py
M	dincli/cli/log.py
A	dincli/sdk/log.py
M	dincli/cli/utils.py
M	dincli/services/cid_utils.py
M	dincli/services/ipfs.py
A	dincli/sdk/config.py
A	dincli/sdk/errors.py
A	dincli/sdk/manifest.py
A	dincli/sdk/runtime.py
A	dincli/sdk/serialize.py
A	dincli/sdk/state.py
A	dincli/sdk/web3.py
M	dincli/services/runtime.py
M	tests/test_connect_wallet.py
M	tests/test_ipfs_config.py
A	tests/test_sdk_boundary.py
A	tests/test_sdk_config.py
A	tests/test_sdk_manifest.py
A	tests/test_sdk_serialize.py
A	tests/test_sdk_state.py
A	tests/test_sdk_web3.py

What's good

• state.py GI-state converters are byte-identical to the pre-move code. Diffed dincli/sdk/state.py's stateDescription/states/GIstateToDes/GIstateToStr/GIstatestrToIndex against git show origin/develop:dincli/cli/utils.py (lines 565-642) — only whitespace differs. Genuinely verbatim, as claimed.
• The new GIState IntEnum's ordinals match Solidity exactly. Checked foundry/src/DINShared.sol's enum GIstates { AwaitingDINTaskAuditorToBeSet, // 0 ... GIended // 22 } (23 members, 0-22) against the states list in state.py — same 23 names, same order, index-for-index. hardhat/contracts/DINShared.sol matches too (both toolchains use the identical enum).
• The context.py delegation preserves exit-code/message semantics. validate_GIstate_ET_given_GIstate and validate_GIstate_LTE_given_GIstate now call validate_gi_state_equals/validate_gi_state_at_least and catch ValidationError, but the console.print(f"[bold red]✗ {msg}. Current state: ...[/bold red]") + raise typer.Exit(1) fallback is untouched and fires on the same conditions as the pre-extraction inline checks. Traced both sides; logic is equivalent.
• validation_failed's detail allowlist matches what state.py actually raises. _ALLOWLIST["validation_failed"] = {"field": "str", "expected": "str", "actual": "str"} in errors.py lines up exactly with the details={"field": ..., "expected": ..., "actual": ...} dict both validate_gi_state_equals and validate_gi_state_at_least construct — nothing gets silently dropped by sanitize_details.
• serialize.py's encoder rules all work correctly on the edge cases I tried by hand (not just the happy-path tests already in the file): nested dataclasses, a list of dataclasses, uint256 at 2**256-1 (Python's arbitrary-precision int means no truncation regardless of value size), zero, and tagged-None fields all serialize correctly. See finding No. 3 for the one edge case that isn't clean.
• The import-boundary test is real and actually covers the new modules, not just the old ones. tests/test_sdk_boundary.py::test_sdk_imports_no_cli_or_ui runs in a fresh subprocess, walks every submodule under dincli.sdk via pkgutil.walk_packages, and explicitly asserts {"dincli.sdk.state", "dincli.sdk.serialize"} <= loaded before checking neither typer/rich nor any dincli.cli.* module got pulled in. This is exactly the test Umer asked for, and it's not vacuous — I ran it standalone and it passes.
• Full test suite passes: 144 passed (torch-gated tests/test_cache_client_dp.py and the live-chain tests/dincli/ directory excluded per the task brief — both need dependencies not present in this venv). This matches the PR body's claimed count. The SDK-focused subset (test_sdk_state.py, test_sdk_serialize.py, test_sdk_boundary.py, test_sdk_manifest.py, test_sdk_config.py, test_sdk_web3.py) is 70 of those 144 and all pass on their own.
• Two of the three loader-tier shims I spot-checked in depth (dincli/services/runtime.py, dincli/services/cid_utils.py) are genuinely behavior-identical — diffed them directly against dincli/sdk/runtime.py/dincli/sdk/cid.py and the only differences are import paths and a docstring header.

Findings

No. 1 — systemic, repeated departure from the "zero behavior change" claim: every console.print in code that moved into dincli/sdk/* silently became a logger.* call. Non-blocking, but should be called out explicitly rather than left implied by "zero behavior change." ----- ** No Action Needed Santigo - Just flagging what behaviour changed **

This isn't a one-off — it's the same pattern in at least three places across the loader-tier commits:

1. dincli/sdk/ipfs.py::upload_to_ipfs — the three provider-branch status lines that were console.print("[bold green]Uploading via Environment-backed IPFS...[/bold green]") (and the Filebase/custom equivalents) in the pre-move dincli/services/ipfs.py are now logger.info("Uploading via Environment-backed IPFS..."). Confirmed by diffing git show origin/develop:dincli/services/ipfs.py against dincli/sdk/ipfs.py — every other message in that file already used logger.* before the move; only these three were rich-styled console.print calls, and only these three changed character.
2. dincli/sdk/manifest.py::get_manifest — the freshness-check warning, previously console.print(f"[yellow]Warning: Could not verify manifest freshness: {e}[/yellow]") in cache_manifest's ancestor code (git show origin/develop:dincli/cli/utils.py around line 750), is now logger.warning("Could not verify manifest freshness: %s", e).
3. dincli/sdk/config.py::get_env_key/set_env_key — the "{key} not found in .env file" (was [bold red]), "python-dotenv not installed" (was [yellow]), and "Error saving to .env" (was [red]) messages are all now logger.warning.

None of these are functional regressions — dincli/sdk genuinely cannot import rich per the architectural constraint this whole PR exists to establish, so some conversion was unavoidable, and the CLI-facing wrapper cache_manifest in cli/utils.py still does its own console.print for the messages that matter for exit-code semantics (verified — see "What's good" above). But three concrete, user-visible things actually changed for these specific lines: they lose rich styling/color, they move from stdout to stderr (Python logging's default StreamHandler target), and they become gate-able by the user's configured log_level — previously these particular console.print calls fired unconditionally regardless of log-level config, so a user who has set log_level: WARNING or higher in their dincli config would now silently lose the "Uploading via..." status lines (INFO-level) that used to always print. At the default log_level: INFO all three still surface, just reformatted.

This is exactly the kind of thing the "zero behavior change, all call sites unchanged" claim in the loader-tier commit messages should not paper over. Recommend either amending those commit messages/PR description to scope the claim to "unchanged call sites and return values" (true) rather than "unchanged behavior" (not quite true for these log/console lines), or explicitly listing this console→logger conversion as a known, intentional side effect of the SDK boundary.

No. 2 — cache_manifest(..., info=True) now makes two on-chain RPC calls (getModel) where the old code made one. Non-blocking (minor inefficiency, not a correctness bug), but worth flagging since it's a real behavior change the "zero behavior change" claim doesn't account for.

Old cache_manifest (single function) fetched model_info = din_registry_contract.functions.getModel(model_id).call() once and used it both for the info print branch and the download/freshness check. The new split (dincli/sdk/manifest.py::download_manifest + get_model_info, glued back together by cli/utils.py::cache_manifest) calls download_manifest() (which may itself call getModel if the manifest is missing/stale) and then, independently, get_model_info() calls getModel again for the info dict. tests/test_sdk_manifest.py::TestCacheManifestCLIWrapper::test_info_flag_calls_get_model_info_and_prints actually asserts this double-call shape (download_calls == [5] and get_model_info_calls == [(5, True)] as two separate mock invocations), so it's tested — just not flagged as a divergence from the original single-RPC-call design. Any dincli ... --info command now does one extra eth_call round-trip. Low severity (RPC calls are cheap and idempotent reads), but real, and it's the kind of thing worth a one-line callout in the PR description rather than leaving under a "zero behavior change" umbrella.

No. 3 — serialize.py's uint256_string and address encoders don't validate their input; a negative int or a malformed address propagates either an unvalidated negative string or a raw ValueError (not a DinError) out of the SDK. Non-blocking follow-up, not a blocker for this PR.

Reproduced both by hand:

>>> _encode_dataclass(Outer(inner=Inner(val=5), items=[...], negative=-42, huge=2**256-1))
{'inner': {'val': '5'}, ..., 'negative': '-42', 'huge': '115792089237316195423570985008687907853269984665640564039457584007913129639935'}

str(int(raw)) happily stringifies a negative number tagged uint256_string — there's no assertion that the value is actually in [0, 2**256). Given these values are expected to always originate from on-chain uint256 reads, this is unlikely to trigger in practice, but if it ever does (a bug upstream, a manually-constructed dataclass in a future daemon handler), the envelope would carry a semantically-invalid "-42" for a field the wire-protocol schema promises is a non-negative uint256 string, with no error raised anywhere.

>>> _encode_dataclass(Addr(a='not_an_address'))
ValueError: when sending a str, it must be a hex string. Got: 'not_an_address'

A malformed address-tagged field raises a bare ValueError from Web3.to_checksum_address, not one of the DinError subclasses this same PR's errors.py establishes as the required shape for anything that crosses the SDK boundary. If to_envelope() is ever called with a dataclass containing a bad address (e.g. from a future daemon handler that didn't validate input upstream), the raw ValueError would propagate uncaught rather than becoming a structured {"code": ..., "message": ..., "details": ...} error — undermining the "every SDK-boundary error should be JSON-shaped" property the design review approved in §4.

Neither of these is exercised by tests/test_sdk_serialize.py — that file is happy-path only for these two encoders (valid, non-negative, well-formed inputs). Test coverage is otherwise decent (uint256 stringification, omit, checksum, enum-to-name, Decimal, nullable-tagged-None, and envelope success/error/null-data/correlation_id are all covered), but these two negative-input paths aren't.

No. 4 — two open asks from the design review (already flagged there, confirmed still unaddressed by this diff) — non-blocking follow-ups, not blockers for this PR.

• Amount-field tagging scan test: the design review (review.md in this folder, §2 close-out) asked for "a test that scans dataclasses for amount-shaped int fields and asserts they're tagged [metadata={"json": "uint256_string"}], same allowlist discipline §4a already applies to error details." No such test exists anywhere in this diff (grep -rln "uint256_string" across the tree hits only dincli/sdk/serialize.py itself and its own test file — no scanner). Still open; reasonable to defer to whenever the first real daemon-facing dataclasses land (this PR only ships the encoder + test fixtures, no production dataclasses to scan yet), but worth a tracking note so it doesn't quietly fall off.
• correlation_id origin: the design review asked for a one-line clarification of where correlation_id comes from (generated per-call? tied to a job id?). Developer/design/sdk-interface-proposal.md is unchanged on this point since that review (still just "optional for CLI, first-class for daemon jobs/logs" with no origin spec), and to_envelope()'s implementation just accepts whatever string the caller passes — it doesn't generate or attach one itself. Still open, still non-blocking (this is presumably PR #32's / the daemon's job to decide).

No. 5 — dincli/sdk/ipfs.py's # TODO(sdk-keystones): IpfsError is mislabeled: it isn't actually blocked on the deferred wallet/session/tx keystone, and it's a real gap for dind's retry logic today. Non-blocking, but should be re-scoped/actioned separately rather than left bundled with the private-key-sensitive work.

_raise_for_http_error (ipfs.py:89-98) and the requests.exceptions.RequestException catches in upload_to_ipfs/retrieve_from_ipfs (ipfs.py:183-184, 252-254) all raise a bare RuntimeError, tagged with a TODO(sdk-keystones): IpfsError comment implying the fix is gated on the same keystone work as wallet.py/session.py/tx.py (dincli/sdk/web3.py:14 has the equivalent TODO(sdk-keystones): NetworkError). But IpfsError already exists and is fully wired into the error taxonomy this same PR ships: errors.py:70 (class IpfsError(DinError): code = "ipfs_error"), and Developer/design/sdk-interface-proposal.md:281 explicitly calls out converting these raw RuntimeErrors into it. There's no dependency on DinSession/SignerProvider here — IpfsError doesn't touch signing at all, it just needs raise IpfsError(...) swapped in for raise RuntimeError(...) in the three spots above (plus an _ALLOWLIST entry in errors.py, e.g. {"provider": "str", "status_code": "int"}, so the detail sanitization the rest of errors.py enforces actually covers it).

This matters concretely for dind (PR #32): per errors.py's own module docstring, "the daemon maps [DinErrors] to job errors and keys retries off the stable code." A failed IPFS upload today raises a RuntimeError with no .code — dind's future job-retry policy has nothing structured to key off for "IPFS upload failed, should this job retry?" vs. any other unexpected exception. Recommend either wiring IpfsError now (it's a small, low-risk change fully within this PR's own architecture) or re-tagging the TODO so it doesn't read as blocked on the keystone task it isn't actually blocked on.

No. 6 — to_envelope() (§3's JSON envelope) has zero production call sites: nothing in either PR actually wires an SDK operation's dataclass return through it yet. Expected sequencing gap, not a regression — flagging so it's tracked explicitly rather than left implicit.

grep -rn "to_envelope" --include="*.py" across both worktrees turns up exactly two hits: the function's own definition in dincli/sdk/serialize.py and its test file, tests/test_sdk_serialize.py. No production code — not dincli/cli/*, not dincli/dind/* (PR #32) — ever calls it. Consistent with that: dincli/dind/jobs.py's JOB_HANDLERS registry has exactly one entry, "demo", and it's a no-op; and there is no --json flag anywhere in dincli/cli/ (grepped, no hits) for the CLI half of §3's "daemon always; CLI on --json" consumer-boundary design. So while to_envelope() itself is correctly built and tested (see "What's good" and No. 3 above), the pipeline that's supposed to produce the dataclasses it consumes — the sdk/operations/ layer from §8's package layout, calling real SDK functions for task registration, GI lifecycle, aggregator/auditor scoring, etc. — doesn't exist in either PR yet.

This is expected, not a bug: operations/ is explicitly one of the modules gated behind the deferred wallet/session/tx keystone (§8, "the remaining seven"), so there's nothing for it to wrap yet. Once that keystone lands and operations/ starts shipping real functions, dind will register real job handlers (client/auditor/aggregator role automation) that call into operations/ and route the resulting dataclasses through to_envelope() for the job queue and /health/logs — replacing today's single no-op "demo" handler. Recommend tracking this explicitly — added as BL-9 in Developer/BACK_LOG.md alongside this review — so "envelope built, nothing consumes it yet" doesn't quietly get lost as an implicit gap between the two PRs.

Recommendation

Mergeable as-is, modulo the deferred wallet/session/tx keystone (explicitly out of scope for this PR). Nothing here is a functional regression that breaks a call site — the four loader-tier shims I checked (cli/utils.py, cli/contract_utils.py, services/ipfs.py, services/runtime.py, services/cid_utils.py) all correctly re-export working code, state.py is verified byte-identical and ordinal-correct against the Solidity enum, and the full suite (144 tests) passes clean.

The one thing I'd push back on before merge is the wording of the commit messages / PR description: "zero behavior change" is not quite accurate for the loader-tier commits (No. 1, No. 2 above are real, if minor, behavior changes) — I'd ask for that language to be softened to something like "unchanged call sites and return contracts; console output for moved SDK-layer functions now routes through logging" so nobody downstream is surprised later. Findings No. 3 and No. 4 are legitimate gaps but genuinely low-stakes follow-up work, not blockers. No. 5 (the IpfsError TODO) is worth a quick fix or at least a re-scope before this lands, since it's cheap, self-contained, and directly feeds dind's retry logic — no reason to let it sit mislabeled as blocked on the keystone task. No. 6 (the unwired envelope/operations/ layer) isn't a blocker either — it's the natural next layer once the keystone lands, tracked in the backlog rather than left implicit.