feat: --custody managed + local SDK pin for ENGN-8646 testing - #34
feat: --custody managed + local SDK pin for ENGN-8646 testing#34TaniBuilds wants to merge 12 commits into
Conversation
Lets a worker run with Privy-managed custody (auto-provision a wallet bound to
its topic) and migrates worker_runtime to the branch SDK's worker API:
- worker_runtime.py: add --custody {local,managed}; managed builds
AlloraWalletConfig.from_env() (FORGE_API_KEY → provision per-topic, no local
key). Migrate to AlloraWorker.inferer(...) and adapt the run fn to the
RunContext-based signature (raw_fn(ctx.nonce)).
- pyproject.toml: LOCAL-DEV [tool.uv.sources] pin of allora-sdk to the local
branch worktree (carries the unreleased auto-provision changes). Remove and
bump the version once the SDK release ships.
🔍 Multi-Agent Multi-Repo Code Review 🧑💻16 findings across 9 dimensions — 🟡 4 medium · 🔵 7 low · ⚪ 5 info 🧵 Synthesized by Dimensions: ✅ Correctness (6 findings)🟡 synth-001 (medium) —
Consequences:
The three 💡 Add a guard in 🔗 Cross-repo: affects allora-sdk-py 🔵 synth-005 (low) — 🤝 flagged by 3 agents: correctness, deep-analysis, adversarial
1. Name-based dispatch false-positives (correctness-2, DA-001): Parameters named 2. Substring annotation match (adv-3): 3. Class-based artifacts (adv-6): 🎭 adversarial note: Phase B (adv-1) challenged DA-001's HIGH severity as inflated (matching correctness-2's LOW) and warned that DA-001's proposed execution-based probe would replace a safe static check with a side-effect-prone runtime probe of untrusted cloudpickle-deserialized code — double-executing non-idempotent artifacts and introducing broad TypeError/AttributeError catch-all logic. The current static inspect.signature approach is architecturally correct; only the matching logic needs tightening. Severity kept at LOW. 💡 Apply three targeted fixes: (1) Tighten the annotation match from substring to exact-token: 🔵 synth-006 (low) — 🤝 flagged by 3 agents: correctness, architecture, style
The companion Additionally, 💡 Either (a) export a 🔵 synth-007 (low) — 🤝 flagged by 2 agents: correctness, style
The two adjacent managed-custody guards (lines 164-168 and 170-174) use 💡 Either escalate to 🔵 synth-009 (low) —
Given the entire PR's correctness rests on this dispatch being wired correctly to the SDK's RunContext-callback contract (cross-repo verified), the missing coverage is a notable gap. The heuristic tests give confidence that 💡 Extract the dispatch from ⚪ synth-012 (info) —
💡 Add: 🔒 Security (2 findings)🟡 synth-003 (medium) — 🤝 flagged by 2 agents: security, adversarial
The kit's only managed-custody guard is 🎭 adversarial note: Phase B (adv-5) challenged sec-001's HIGH severity: cross-repo agent verified the code IS correct against the actual pinned ENGN-8646 branch — 'FORGE_API_KEY always takes precedence' is accurate there. The threat model driving HIGH severity is 'wrong SDK branch resolved by the path pin', which is a build-config gap, not a code bug. Treated as defense-in-depth at MEDIUM. The primary remediation should be replacing the relative path pin with a git+SHA reference (synth-002/synth-015); the kit-side env-var rejection guard is belt-and-suspenders. 💡 Add a defensive guard in 🔗 Cross-repo: affects allora-sdk-py 🟡 synth-004 (medium) —
This is a pre-existing pattern not introduced by this PR, but is now load-bearing for the new managed-custody flow since 💡 Resolve the key-file path against the kit's install location ( 🏗️ Architecture (2 findings)🟡 synth-002 (medium) —
The 💡 Bump the 🔗 Cross-repo: affects allora-sdk-py ⚪ synth-016 (info) —
Architecturally, the legacy-nonce→RunContext adapter belongs in the SDK (which owns the 💡 Open a follow-up on allora-sdk-py to expose a 🔗 Cross-repo: affects allora-sdk-py 🎨 Style (4 findings)🔵 synth-008 (low) —
💡 Collapse into a single block: 🔵 synth-010 (low) —
💡 Tighten annotations: ⚪ synth-013 (info) —
💡 Update help to: ⚪ synth-014 (info) —
💡 Update the docstring to say: "named exactly as one of: 🔗 Cross Repo (2 findings)🔵 synth-011 (low) —
Not a blocker for this PR (the builder-kit only consumes the Python SDK), but if the product later needs Go or TS workers with managed custody and no pre-registered wallet_id, ENGN-8646 equivalent work would need to land on 💡 Track as a follow-up ticket: if managed custody is needed for Go/TS workers, port the 🔗 Cross-repo: affects allora-sdk-go, allora-sdk-ts, allora-sdk-py ⚪ synth-015 (info) — 🤝 flagged by 2 agents: cross-repo, architecture
The cross-repo agent verified directly against the ENGN-8646 branch that all builder-kit contract surfaces are consistent (from_env() deferred-managed path, RunContext.nonce, init_worker_wallet with topic_id, etc.). The prior pass's Until the manifest is updated, every automated review of this PR will produce the same false positives from agents that inspect the wrong sibling branch. 💡 Update the cross-repo sibling manifest to include 🔗 Cross-repo: affects allora-sdk-py 📝 Agent Summaries🎭 adversarial: Six adversarial findings: two challenges to Phase A (DA-001 HIGH severity is inflated and its suggested fix introduces execution-side-effect bugs not present in the current code; sec-001 HIGH conflates a code-contract claim with a build-configuration gap, where the code is actually correct against the documented pinned branch), and four issues Phase A missed. The largest gap is adv-2: the new 🏗️ architecture: The PR's operational/correctness surface was thoroughly covered by the prior pass (blocking I/O moved to sync ✅ correctness: Prior review thoroughly covered the major correctness risks (managed-custody happy-path / SDK pin / blocking from_env in async / dual-mode artifact dispatch / Literal typing) and the author's commits 07c525c, ff77d4a, e5fd3e4, b933c1d, da015ff, d391b1d address them. Remaining issues are residual / smaller: (1) the FEE_GRANTER notification uses warnings.warn rather than a hard parser.error, which is inconsistent with the two adjacent managed-custody guards and can be silently suppressed by parent-process warning filters; (2) the 🔗 cross-repo: Cross-repo verification against the ACTUALLY pinned SDK branch (tani/engn-8646-auto-provision-managed-wallets, allora-sdk-py PR #81) confirms the builder-kit's managed-custody usage is contract-consistent: from_env() deferred-managed path, AlloraWorker.inferer() with topic_id for init_worker_wallet provisioning, RunContext.nonce, and ForgeBackendClient.provision_wallet/clear_association all match. No contract breaks. The prior synth-001 CRITICAL and synth-011 LOW findings were false positives caused by the sibling manifest capturing the wrong branch (ENGN-8615 instead of ENGN-8646) — the manifest gap is the real issue (xrepo-001, medium). Secondary findings: ENGN-8646 managed custody is Python-SDK-only (Go/TS lack an equivalent — parity gap to track, xrepo-003, low); the [tool.uv.sources] path pin silently resolves to the wrong SDK if the sibling worktree is on the wrong branch, with no automated check (xrepo-004, low). The ADR-036 wallet-link device flow (contract A) is untouched. 🔬 deep-analysis: PR improves managed custody wiring and SDK alignment, but the artifact call-shape heuristic introduces a backwards-compatibility regression risk for legacy single-argument functions with context-like parameter naming. ⚡ performance: Performance review of PR #34 (feat: --custody managed + local SDK pin for ENGN-8646 testing). The diff is small and the per-cycle hot path (the inner run_fn closure driving worker.run()) is effectively unchanged: call-shape detection is cached once at load time, and the loop body is a float cast + finite check. The previously-flagged blocking from_env() HTTPS call has been correctly moved out of async _run() into sync main() before asyncio.run(), so it no longer stalls the event loop. The only new per-cycle overhead introduced by this repo's diff is the switch from AlloraWorker(...) to AlloraWorker.inferer(...), which enables the SDK's default SanityCheck and adds a per-submission consensus-fetch RPC; this is SDK-default behavior already documented under the prior correctness finding synth-013, flagged here only for the performance dimension. All other new work is startup-only. No critical, high, or medium performance defects introduced by this repo's diff; remaining perf-relevant surface (managed-wallet get-or-create memoization, per-nonce RPC behavior) is owned by the sibling allora-sdk-py branch. 🔄 runtime-semantics: No runtime-semantics defects identified in this diff; custody routing, callback adaptation, and async worker invocation appear consistent with intended behavior. 🔒 security: Three security findings. The primary one (sec-001, HIGH) re-opens the credential-confusion concern that the author previously dismissed as a false positive: the 'no silent local-key fallback' docstring claim only holds on the sibling allora-sdk-py ENGN-8646 branch, but the 🎨 style: The PR introduces 📋 synthesis: Pass 1 synthesis for PR #34 (--custody managed + local SDK pin for ENGN-8646 testing). The prior-state shows synth-001 through synth-015 were already posted in a prior pass; the author addressed the CRITICAL (confirmed false positive — wrong sibling branch in manifest) and acknowledged the HIGH (uv.sources pin intentional for draft). Several prior issues are now fixed in the PR HEAD: blocking from_env() moved to sync main(), parser.error guards for managed+mnemonic and missing FORGE_API_KEY added, _artifact_expects_context detection added, run_fn properly typed, Literal typing used in _resolve_wallet_cfg. New findings in this pass (not in prior state): The most important missed issue is synth-001 (MEDIUM): the Generated by code-review-forge — multi-agent PR review with temporal redundancy |
TaniBuilds
left a comment
There was a problem hiding this comment.
🔍 Multi-Agent Inline Review
12 inline findings posted as resolvable threads.
3 findings not on changed lines (see PR comment for full report): 🟡 1 medium, 🔵 2 low
Generated by code-review-forge
…t loop Managed custody calls AlloraWalletConfig.from_env(), which performs a blocking wallet-info HTTPS fetch. Doing it inside the async _run() stalled the event loop at startup (and delayed cancellation if the backend hung). Move resolution into a sync _resolve_wallet_cfg() helper invoked from main() before asyncio.run(), and pass the pre-resolved wallet_cfg into _run(). Addresses synth-003.
…ssion drops run_fn unconditionally called raw_fn(ctx.nonce). An artifact written to the newer RunContext contract (raw_fn(ctx)) would get an int, raise AttributeError inside Inferer.submit, and be silently caught — dropping every submission with no alarm. Resolve the artifact's call shape once at load via inspect.signature (defaulting to the legacy nonce form for back-compat) and document the supported artifact contract in _run's docstring. Addresses synth-005.
The inferer callback closure had no parameter or return annotation. Import RunContext under a TYPE_CHECKING guard and annotate run_fn(ctx: RunContext) -> float so pyright catches call-site type errors (e.g. a future ctx.nonce rename). Addresses synth-009.
The custody routing parameter only ever takes two values. Narrow it from a bare str to Literal["local", "managed"] (the resolution moved into _resolve_wallet_cfg) per the strong-typing mandate, so direct callers and pyright catch invalid values instead of silently falling through. argparse choices still enforce the CLI boundary. Addresses synth-008.
Passing --custody managed --mnemonic-file silently discarded the key file (managed custody resolves the wallet from FORGE_API_KEY), leaving an operator to believe a local key was in play. Surface the conflict as a parser.error before any network calls. Addresses synth-004.
Fail fast with a clear usage error when --custody managed is used without FORGE_API_KEY, naming the missing variable instead of surfacing it deep in the SDK. Also align the _resolve_wallet_cfg docstring with the real auto-provision SDK contract: FORGE_API_KEY (no FORGE_SIGNING_WALLET_ID) yields a deferred managed config and takes precedence over PRIVATE_KEY/MNEMONIC, so there is no silent local-key fallback. Hardening for synth-001 (false positive against the wrong SDK branch; see PR reply).
A managed (Privy) wallet never receives faucet drips, so gasless submission requires a fee granter; without FEE_GRANTER every transaction fails with 'insufficient fees'. Emit a startup warning naming the variable. Note: unlike the reviewer's claim, FORGE_SIGNING_WALLET_ID is NOT required — the auto-provision SDK defers wallet creation when only FORGE_API_KEY is set. Addresses synth-006.
Add a module docstring clarifying that ALLORA_API_KEY (faucet/consumer, via --api-key) and FORGE_API_KEY (managed-custody backend) are different secrets for different backends, and that --custody managed reads FORGE_API_KEY only. Reduces the naming-collision footgun. Addresses synth-012.
…patch Add pytest cases asserting custody='managed' routes to AlloraWalletConfig.from_env() without reading mnemonic_file, custody='local' builds AlloraWalletConfig(mnemonic_file=...) without calling from_env(), the managed+mnemonic-file and managed-without-FORGE_API_KEY CLI guards exit, and _artifact_expects_context distinguishes legacy nonce vs RunContext artifacts. Addresses synth-007.
…erer The raw example still used the removed AlloraWorker(...) direct constructor with a predict_fn(nonce) signature, which would break against the branch SDK this PR targets. Mirror worker_runtime.py: switch to AlloraWorker.inferer(...) and wrap the pickled nonce fn in a RunContext adapter (_run_fn). Addresses synth-010.
Automated-review triage — actionedWorked through all 12 unresolved inline threads plus the 3 off-line findings (synth-007/010/012). All 12 threads are now resolved with replies. One commit per issue; the local-dev A) Fixed (commit → finding)
B) Declined / no code change (with reason)
C) Needs user attention
D) Verification
|
TaniBuilds
left a comment
There was a problem hiding this comment.
🔍 Multi-Agent Inline Review
16 inline findings posted as resolvable threads.
Generated by code-review-forge
| @@ -77,16 +150,43 @@ def main() -> None: | |||
| parser.add_argument("--mnemonic-file", default=None, help="Path to wallet key file (managed by WorkerManager)") | |||
There was a problem hiding this comment.
🟡 MEDIUM · ✅ correctness · synth-001
--custody local default + no --mnemonic-file → SDK getpass() hangs headless workers or silently creates an unfunded wallet
When --custody local (the default) is used without --mnemonic-file, _resolve_wallet_cfg('local', None) returns None. The ENGN-8646 SDK's init_worker_wallet(None, topic_id) falls through to: (a) read .allora_key from CWD if present, OR (b) call getpass('Mnemonic: ') for interactive input, AND if the operator presses Enter, silently generates a fresh mnemonic and writes it to .allora_key in CWD.
Consequences:
- In headless/supervised mode (WorkerManager, systemd, Docker without TTY), the worker hangs indefinitely on
getpass. Operators see a worker process that never starts producing submissions. - If
getpasssucceeds with empty input, the SDK creates a brand-new, unfunded wallet the operator did not consciously create. Combined with--no-faucet, the worker signs with a zero-balance wallet and every tx fails. - The
.allora_keyCWD path is easy to confuse with the kit's_load_api_keyfallback paths.
The three --custody managed guards in main() use parser.error() for strict pre-flight validation; the --custody local path has no symmetric guard. No Phase-A agent flagged this — the adversarial reviewer caught it by inspecting the ENGN-8646 SDK branch directly (utils.py lines 47-64).
| parser.add_argument("--mnemonic-file", default=None, help="Path to wallet key file (managed by WorkerManager)") | |
| if args.custody == "local" and not args.mnemonic_file and not os.path.exists(".allora_key"): | |
| parser.error( | |
| "--custody local requires --mnemonic-file or an existing .allora_key in CWD; " | |
| "otherwise the SDK falls back to an interactive getpass() prompt and hangs in headless mode" | |
| ) |
🔗 Cross-repo: affects allora-sdk-py
| @@ -42,3 +42,12 @@ Homepage = "https://github.com/allora-network/allora-forge-builder-kit" | |||
|
|
|||
There was a problem hiding this comment.
🟡 MEDIUM · 🏗️ architecture · synth-002
allora-sdk>=1.0.6 dependency floor does not gate on AlloraWorker.inferer — clean pip install . fails at runtime
The PR switches from AlloraWorker(run=run_fn, ...) to AlloraWorker.inferer(run=run_fn, ...) (worker_runtime.py line 133, notebooks/deploy_worker_raw.py line 64). The .inferer() classmethod factory exists on the sibling tani/engn-8646-auto-provision-managed-wallets branch but is absent from the published allora-sdk==1.0.6 release on PyPI.
The [tool.uv.sources] block hides this for the author's machine (uv resolves the local worktree), but a clean pip install . — which ignores [tool.uv.sources] — resolves allora-sdk==1.0.6 from PyPI and worker_runtime.py fails at the first worker construction with AttributeError: type object 'AlloraWorker' has no attribute 'inferer'. Any downstream user, GitHub Actions step using pip, or wheel build is affected. The declared dependency floor is therefore wrong for the actual API surface the kit now uses.
💡 Suggestion: Bump the allora-sdk dependency floor in [project].dependencies to the first release that ships AlloraWorker.inferer and the deferred-managed from_env() path. Until that release exists, mark the kit version as a pre-release and document in the README that uv + the sibling worktree pin is mandatory for installation. Add a CI matrix step that installs via plain pip install . (no uv, no local path) to catch floor regressions.
🔗 Cross-repo: affects allora-sdk-py
| return cfg | ||
|
|
||
|
|
||
| def _resolve_wallet_cfg( |
There was a problem hiding this comment.
🟡 MEDIUM · 🔒 security · synth-003
Kit lacks defensive guard rejecting PRIVATE_KEY/MNEMONIC/MNEMONIC_FILE under managed custody — wrong-SDK-branch resolution causes silent credential confusion
🤝 flagged by 2 agents: security, adversarial
_resolve_wallet_cfg documents "FORGE_API_KEY always takes precedence — there is no silent local-key fallback." This claim is correct ONLY on the tani/engn-8646-auto-provision-managed-wallets branch (confirmed by cross-repo agent via direct inspection). However, the [tool.uv.sources] pin is a CWD-relative path with no branch or SHA constraint. If a reviewer, CI machine, or downstream consumer has the wrong SDK branch checked out (tani/ENGN-8615-sdk-privy-delegated-signing or any released SDK without ENGN-8646), AlloraWalletConfig.from_env() falls through to read PRIVATE_KEY/MNEMONIC/MNEMONIC_FILE. An operator running --custody managed with stale local-key env vars would silently sign transactions with their local key while believing they are using Privy-managed custody.
The kit's only managed-custody guard is FORGE_API_KEY presence (line 170-174); it does NOT detect or reject conflicting local-key env vars. The --mnemonic-file mutual-exclusion guard catches the explicit flag but not the env vars that from_env() reads.
🎭 adversarial note: Phase B (adv-5) challenged sec-001's HIGH severity: cross-repo agent verified the code IS correct against the actual pinned ENGN-8646 branch — 'FORGE_API_KEY always takes precedence' is accurate there. The threat model driving HIGH severity is 'wrong SDK branch resolved by the path pin', which is a build-config gap, not a code bug. Treated as defense-in-depth at MEDIUM. The primary remediation should be replacing the relative path pin with a git+SHA reference (synth-002/synth-015); the kit-side env-var rejection guard is belt-and-suspenders.
| def _resolve_wallet_cfg( | |
| def _resolve_wallet_cfg( | |
| custody: Literal["local", "managed"], mnemonic_file: str | None | |
| ) -> AlloraWalletConfig | None: | |
| if custody == "managed": | |
| # Fail closed: under any SDK without the ENGN-8646 deferred-managed path, | |
| # from_env() would silently fall through to PRIVATE_KEY/MNEMONIC/MNEMONIC_FILE. | |
| # Reject those env vars explicitly so managed custody can never sign with a local key. | |
| stale = [v for v in ("PRIVATE_KEY", "MNEMONIC", "MNEMONIC_FILE") if os.environ.get(v)] | |
| if stale: | |
| raise RuntimeError( | |
| "--custody managed refuses to proceed with local-key env vars set " | |
| f"({', '.join(stale)}); clear them or use --custody local" | |
| ) | |
| return AlloraWalletConfig.from_env() | |
| return AlloraWalletConfig(mnemonic_file=mnemonic_file) if mnemonic_file else None |
🔗 Cross-repo: affects allora-sdk-py
| from allora_sdk.worker.context import RunContext | ||
|
|
||
|
|
||
| def _load_api_key(explicit: str | None) -> str: |
There was a problem hiding this comment.
🟡 MEDIUM · 🔒 security · synth-004
_load_api_key falls back to CWD-relative key files with no ownership check
_load_api_key (lines 31-41) falls back to os.path.exists / open on "notebooks/.allora_api_key" and ".allora_api_key" — both CWD-relative with no canonicalization or file-ownership check. A worker started in an attacker-writable scratch directory silently reads an attacker-supplied consumer key, causing the worker to authenticate to the Allora consumer/faucet API under an attacker-chosen identity (polluting attribution, draining the attacker's faucet quota, or confusing inference records). Under --custody managed, where the operator may have deliberately avoided local key material, this CWD fallback still loads an implicit key from disk without any indication.
This is a pre-existing pattern not introduced by this PR, but is now load-bearing for the new managed-custody flow since ALLORA_API_KEY is still required even under --custody managed.
💡 Suggestion: Resolve the key-file path against the kit's install location (os.path.dirname(__file__)) rather than CWD, and verify the file is not world-writable (os.stat mode check) before reading. Or remove the file fallback entirely and require ALLORA_API_KEY / --api-key explicitly.
| return AlloraWalletConfig(mnemonic_file=mnemonic_file) if mnemonic_file else None | ||
|
|
||
|
|
||
| def _artifact_expects_context(raw_fn: Callable[..., object]) -> bool: |
There was a problem hiding this comment.
🔵 LOW · ✅ correctness · synth-005
_artifact_expects_context has three distinct misclassification modes: name heuristic, substring annotation match, and class artifacts
🤝 flagged by 3 agents: correctness, deep-analysis, adversarial
Three failure modes in _artifact_expects_context (lines 76-95):
1. Name-based dispatch false-positives (correctness-2, DA-001): Parameters named ctx, context, or run_context trigger RunContext dispatch regardless of type. A legacy artifact def predict(ctx): return float(ctx) * scale — where ctx is the author's name for the integer nonce — will receive a RunContext object and raise TypeError / AttributeError on every inference cycle. This appears as silent topic non-participation because the SDK's Inferer catches and logs the error per submission without raising. The universe of pre-existing artifacts using these param names is small but non-zero (cloudpickle preserves source names verbatim).
2. Substring annotation match (adv-3): "RunContext" in annotation matches MyRunContext, FastRunContextWrapper, WorkerRunContextV2, and any user-defined class containing "RunContext" as a substring. Such artifacts would receive an SDK RunContext instance instead of their custom type. Fix: exact-token match on the trailing qualified-name component.
3. Class-based artifacts (adv-6): inspect.signature on a class returns the __init__ signature, not __call__. A pickled class whose __init__ takes a param named ctx would trigger false-positive RunContext dispatch — constructing a new instance per nonce instead of calling the intended prediction entry point. Tests do not cover the class case.
🎭 adversarial note: Phase B (adv-1) challenged DA-001's HIGH severity as inflated (matching correctness-2's LOW) and warned that DA-001's proposed execution-based probe would replace a safe static check with a side-effect-prone runtime probe of untrusted cloudpickle-deserialized code — double-executing non-idempotent artifacts and introducing broad TypeError/AttributeError catch-all logic. The current static inspect.signature approach is architecturally correct; only the matching logic needs tightening. Severity kept at LOW.
| def _artifact_expects_context(raw_fn: Callable[..., object]) -> bool: | |
| annotation = "" if params[0].annotation is inspect.Parameter.empty else str(params[0].annotation) | |
| # Match the exact short name (handles forward-ref strings, qualified names, and quoted strings) | |
| # to avoid matching MyRunContext, FastRunContextWrapper, etc. | |
| short = annotation.strip("'\"").rsplit(".", 1)[-1] | |
| return short == "RunContext" or params[0].name in ("ctx", "context", "run_context") |
| @@ -77,16 +150,43 @@ def main() -> None: | |||
| parser.add_argument("--mnemonic-file", default=None, help="Path to wallet key file (managed by WorkerManager)") | |||
There was a problem hiding this comment.
⚪ INFO · ✅ correctness · synth-012
--api-key argument missing help text — invites confusion between ALLORA_API_KEY and FORGE_API_KEY under --custody managed
parser.add_argument("--api-key", default=None) has no help= string (line 149). The module docstring carefully distinguishes ALLORA_API_KEY (consumer/faucet) from FORGE_API_KEY (Forge backend, managed wallet), but --help shows --api-key API_KEY with no description. An operator running --custody managed who sees an "ALLORA_API_KEY not found" error from _load_api_key may incorrectly supply their FORGE_API_KEY via --api-key, causing a 401 from the topic-query API or leaking the Forge key in shell history.
| parser.add_argument("--mnemonic-file", default=None, help="Path to wallet key file (managed by WorkerManager)") | |
| parser.add_argument("--api-key", default=None, help="Allora consumer API key (ALLORA_API_KEY) for faucet/topic queries — distinct from FORGE_API_KEY used by --custody managed") |
| @@ -77,16 +150,43 @@ def main() -> None: | |||
| parser.add_argument("--mnemonic-file", default=None, help="Path to wallet key file (managed by WorkerManager)") | |||
There was a problem hiding this comment.
⚪ INFO · 🎨 style · synth-013
--mnemonic-file help text is stale with respect to the new --custody option
--mnemonic-file help says "Path to wallet key file (managed by WorkerManager)" (line 150). With --custody local now exposed as the default, this flag is the user-facing way to point a self-custodial worker at a key file — not exclusively a WorkerManager-internal concern. The word "managed" also conflicts with --custody managed (Privy-managed wallets vs WorkerManager-managed), and the new error message says --mnemonic-file is incompatible with --custody managed, which confuses the two senses of "managed".
| parser.add_argument("--mnemonic-file", default=None, help="Path to wallet key file (managed by WorkerManager)") | |
| parser.add_argument("--mnemonic-file", default=None, help="Path to wallet mnemonic file (used with --custody local; mutually exclusive with --custody managed)") |
| return AlloraWalletConfig(mnemonic_file=mnemonic_file) if mnemonic_file else None | ||
|
|
||
|
|
||
| def _artifact_expects_context(raw_fn: Callable[..., object]) -> bool: |
There was a problem hiding this comment.
⚪ INFO · 🎨 style · synth-014
_artifact_expects_context docstring does not enumerate the accepted context-indicator parameter names
The docstring (lines 77-83) says the function switches when the parameter is "named like a context" but never lists the accepted names. The actual implementation accepts exactly ("ctx", "context", "run_context") (line 95). context in particular is extremely common in Python (SSL context, logging context, Flask/Django request context, etc.). Without listing these names, developers building new artifacts cannot predict which parameter names will trigger RunContext dispatch — the misclassification risk documented in synth-005 is invisible from the public docstring.
💡 Suggestion: Update the docstring to say: "named exactly as one of: ctx, context, or run_context" and note that any other name (e.g. nonce, value, i) keeps the legacy fn(nonce: int) dispatch.
| # uv picks this up on `uv sync` / `uv run`. REMOVE this block and bump the | ||
| # `allora-sdk` version in [project].dependencies once the SDK release with ENGN-8646 | ||
| # ships; the relative path assumes the allora-sdk-py worktree is a sibling directory. | ||
| [tool.uv.sources] |
There was a problem hiding this comment.
⚪ INFO · 🔗 cross-repo · synth-015
Cross-repo sibling manifest captured the wrong allora-sdk-py branch — causes false-positive reviews
🤝 flagged by 2 agents: cross-repo, architecture
The sibling manifest matched allora-sdk-py@tani/ENGN-8615-sdk-privy-delegated-signing, and the captured diff is that branch's diff. The PR's [tool.uv.sources] pin resolves allora-sdk to ../allora-sdk-py, whose worktree carries tani/engn-8646-auto-provision-managed-wallets (allora-sdk-py PR #81) — a superset with the deferred-managed from_env() path, provision_wallet, clear_association, and AlloraWorker.inferer.
The cross-repo agent verified directly against the ENGN-8646 branch that all builder-kit contract surfaces are consistent (from_env() deferred-managed path, RunContext.nonce, init_worker_wallet with topic_id, etc.). The prior pass's synth-001 CRITICAL and synth-011 LOW findings were false positives caused by this manifest mismatch.
Until the manifest is updated, every automated review of this PR will produce the same false positives from agents that inspect the wrong sibling branch.
💡 Suggestion: Update the cross-repo sibling manifest to include allora-sdk-py@tani/engn-8646-auto-provision-managed-wallets as the primary sibling (or in addition to ENGN-8615). Add a comment in the [tool.uv.sources] block naming the exact branch: # branch: tani/engn-8646-auto-provision-managed-wallets (allora-sdk-py PR #81).
🔗 Cross-repo: affects allora-sdk-py
| return AlloraWalletConfig(mnemonic_file=mnemonic_file) if mnemonic_file else None | ||
|
|
||
|
|
||
| def _artifact_expects_context(raw_fn: Callable[..., object]) -> bool: |
There was a problem hiding this comment.
⚪ INFO · 🏗️ architecture · synth-016
Signature dispatch reverse-engineers SDK call conventions by string-matching instead of consuming a stable SDK adapter
_artifact_expects_context (lines 76-95) decides how to invoke the pickled artifact by string-matching parameter names and annotation strings — reverse-engineering the SDK's TRunFn = Callable[[RunContext], ...] contract. This is fragile against: (a) the SDK renaming RunContext; (b) alternate context types the SDK may add (e.g., ReputerContext already exists in reputer.py:31); (c) artifacts whose param is named ctx but expects a different type.
Architecturally, the legacy-nonce→RunContext adapter belongs in the SDK (which owns the RunContext type and the TRunFn contract) as a documented wrap_legacy_nonce_fn / from_legacy_nonce_fn helper, not in a downstream kit doing reflective introspection on cloudpickled callables. The SDK already has make_reputer_function in worker/utils.py as precedent. The kit has no compile-time guarantee that its dispatch matches what inferer.submit actually passes.
💡 Suggestion: Open a follow-up on allora-sdk-py to expose a allora_sdk.worker.utils.legacy_nonce_fn(fn: Callable[[int], float]) -> Callable[[RunContext], float] adapter. The kit would call it for legacy artifacts and pass RunContext-typed artifacts through unchanged, delegating contract ownership to the SDK.
🔗 Cross-repo: affects allora-sdk-py
Summary
Enables running a worker with Privy-managed custody (auto-provision a wallet bound to its topic) and wires the builder-kit to a local build of the allora-sdk branch so it can be tested before the SDK is released.
Linear: ENGN-8646.
worker_runtime.py: adds--custody {local,managed}.managedbuildsAlloraWalletConfig.from_env()(FORGE_API_KEY → the SDK provisions a wallet bound to the topic at startup; no local key file). Also migrates to the branch SDK's worker API —AlloraWorker.inferer(...)and aRunContext-based run fn (raw_fn(ctx.nonce)) — since the branch is a newer worker-API generation than the publishedallora-sdk 1.0.6.pyproject.toml: a clearly-marked local-dev[tool.uv.sources]pin ofallora-sdkto the siblingallora-sdk-pyworktree (which carries the unreleased auto-provision changes + generated protobuf code).Not for merge as-is
The
[tool.uv.sources]pin and the worker-API migration assume the unreleased SDK. Remove the pin and bump theallora-sdkversion once the SDK release with ENGN-8646 ships. This branch is for local testing in the meantime.Test plan
py_compilecleanuv run … worker_runtime --helpimports against the local SDK and shows--custody {local,managed}FORGE_API_KEYset, run--custody managed --topic Nand confirm the worker provisions a managed wallet, registers, and submits (gasless whenFEE_GRANTER= master address)Summary by cubic
Adds a
--custody managedmode to run workers with Privy-managed wallets that auto-provision per topic (ENGN-8646), and migrates the builder kit to the branchallora-sdkRunContext-based worker API. Keeps a local-dev pin to theallora-sdkbranch for testing until the release ships.New Features
worker_runtime.py:--custody {local,managed};managedusesAlloraWalletConfig.from_env()withFORGE_API_KEYto provision a wallet bound totopic_id(no local key).AlloraWorker.inferer(...)and aRunContext-basedrunfn; raw notebook updated to match.Bug Fixes
main()via_resolve_wallet_cfgto avoid blocking the event loop; pass pre-resolvedwallet_cfginto_run().fn(nonce)andfn(ctx).FORGE_API_KEYfor managed, reject--mnemonic-filewith managed, and warn whenFEE_GRANTERis unset.Written for commit 7e0e80d. Summary will update on new commits.