Skip to content

fix(cli): declare imported deps, raise Python floor, unblock configure-ipfs, add read-din-per-eth - #97

Merged
umermjd11 merged 1 commit into
InfiniteZeroFoundation:developfrom
Santiagocetran:fix/develop-small-backports
Aug 20, 2026
Merged

fix(cli): declare imported deps, raise Python floor, unblock configure-ipfs, add read-din-per-eth#97
umermjd11 merged 1 commit into
InfiniteZeroFoundation:developfrom
Santiagocetran:fix/develop-small-backports

Conversation

@Santiagocetran

Copy link
Copy Markdown
Collaborator

PR A of the split agreed on #79. Four independent fixes, no behavioural change beyond removing a wallet requirement one command never needed.

Two of the four turned out differently on develop than the main commits they came from, so those are called out below rather than buried.

1. requires-python>=3.12

Measured, not inferred. develop:dincli/cli/dintoken.py:76 uses PEP 701 nested same-type quotes inside an f-string expression. Under a uv-installed CPython 3.11.15:

  File "dintoken.py", line 76
    f"...Minimum stake is {Web3.from_wei(MIN_STAKE, "ether")} DINTokens..."
                                                    ^^^^^
SyntaxError: f-string: unmatched '('

Clean under 3.12.3, and sweeping every file under dincli/ that is the only one affected.

Worth noting this is a different site from the one that justified the same bump on main (aggregator.py:76-77, which doesn't exist here) — so develop needed the floor on its own evidence, not by inheritance.

2. Four imported-but-undeclared dependencies

rich, requests, eth-account and py-multibase were all arriving transitively via typer/web3/py-cid instead of being declared.

rich is the load-bearing one, and it's an install-breaking bug today. At this package's own declared floor of typer>=0.9.0, typer 0.9.0 lists rich only under extra == "all":

typer 0.9.0   rich  -> ['rich>=10.11.0,<14.0.0 ; extra == "all"']   # not a base dep
typer 0.27.1  rich  -> ['rich>=13.8.0']                             # base dep

So a resolver may legitimately install without rich. Reproduced in a clean venv, then confirmed fixed:

before (develop's dep list) : ModuleNotFoundError: No module named 'rich'
after  (this branch's list) : import dincli.main OK

rich is imported in 10 files, so it's a hard startup failure rather than a degraded path.

Correction to the plan: click is not applicable here. I listed 42f19ca under PR A on #79, but nothing on develop imports click — main needs it because cli/core.py:7 does, and that file arrives with the chain-id work. So it belongs with PR B, where it will actually be justified, and I've left it out of this one.

3. configure-ipfs added to the skip-list

It was falling through to get_en_w3_account_console() and demanding a connected wallet it never uses — the body references only ctx.obj.console, load_config and resolve_ipfs_config.

Only this one entry was missing. develop already spells "read-wallet" and "show-index" correctly, so the other two-thirds of c5ed47c is a no-op here.

4. read-din-per-eth

dinPerEth() is owner-mutable and buy spends against whatever rate is live, with no way to read it beforehand. Before this, dinPerEth appeared only in abis/DinCoordinator.json.

Deviation from main: main duplicates this command in both aggregator.py and auditor.py. Here it's one shared helper in dintoken.py with the three dintoken sub-apps delegating to it, exactly as they already do for buy/stake/read-stake. Available at all three call sites:

dincli dintoken read-din-per-eth
dincli aggregator dintoken read-din-per-eth
dincli auditor dintoken read-din-per-eth

A test pins that there is only one implementation site, so it can't quietly get duplicated later.

Tests

114 → 130 passing. 16 added, and two of them are guards I checked can actually fail rather than passing vacuously:

  • the skip-list check catches the underscore-vs-dash dead-entry class — flipping "read-wallet" back to "read_wallet" fails it
  • the dependency check walks every import under dincli/ and fails on any that isn't declared, so this class of bug can't come back silently
  • the PEP 701 check detects exactly one line, dintoken.py:76, with no false positives — if that syntax ever disappears the test says so, rather than leaving the floor unexplained

…configure-ipfs, add read-din-per-eth

PR A of the `develop` backport plan agreed on InfiniteZeroFoundation#79. Four independent fixes, no
behavioural change beyond removing a wallet requirement one command never
needed.

**1. `requires-python` >= 3.12.** `dincli/cli/dintoken.py:76` uses PEP 701
nested same-type quotes inside an f-string expression, which is a SyntaxError
before 3.12. Measured, not inferred, under a `uv`-installed CPython 3.11.15:

    SyntaxError: f-string: unmatched '('

Clean under 3.12.3. Sweeping every file under `dincli/`, that is the only one
affected. Note this is a different site from the one that justified the same
bump on `main` (`aggregator.py:76-77`), which does not exist here.

**2. Four imported-but-undeclared dependencies.** `rich`, `requests`,
`eth-account` and `py-multibase` were all arriving transitively via
typer/web3/py-cid rather than being declared. `rich` is the load-bearing one:
at this package's own declared floor of `typer>=0.9.0`, typer 0.9.0 lists rich
only under `extra == "all"`, so a resolver may legitimately install without it.
Reproduced in a clean venv, then confirmed fixed:

    before (develop's dep list) : ModuleNotFoundError: No module named 'rich'
    after  (this branch's list) : import dincli.main OK

`rich` is imported in 10 files, so that is a hard startup failure, not a
degraded path. This is the same class of bug as `main`'s explicit `click`
declaration, but note `click` itself is NOT applicable here: nothing on
`develop` imports it (`main` needs it because `cli/core.py:7` does). It becomes
necessary here only when the chain-id work lands, so it belongs with PR B.

**3. `configure-ipfs` added to the system callback skip-list.** It was falling
through to `get_en_w3_account_console()` and demanding a connected wallet it
never uses: the function body references only `ctx.obj.console`, `load_config`
and `resolve_ipfs_config`. Only this one entry was missing here - `develop`
already spells `"read-wallet"` and `"show-index"` correctly, so the other
two-thirds of `main`'s `c5ed47c` is a no-op on this branch.

**4. `read-din-per-eth`.** `dinPerEth()` is owner-mutable and `buy` spends
against whatever rate is live, with no way to read it first. `dinPerEth`
previously appeared only in `abis/DinCoordinator.json`.

Deviation from `main` worth flagging: `main` duplicates this command in both
`aggregator.py` and `auditor.py`. Here it is one shared helper in
`dintoken.py`, with the three `dintoken` sub-apps delegating to it exactly as
they already do for buy/stake/read-stake. A test pins that there is only one
implementation site.

16 tests added, including two guards proven able to fail by mutation: the
skip-list check catches the underscore-vs-dash dead-entry class, and the
dependency check catches any future import that is not declared. Suite:
114 -> 130 passing.

Refs InfiniteZeroFoundation#79
@umermjd11

Copy link
Copy Markdown
Collaborator

Reviewed against unmodified develop — reproduced each of the four fixes rather than just reading the diff, using a real Python 3.11.15 interpreter (via uv) and an isolated venv for the dependency-resolution claim, matching the PR's own verification method.

1. requires-python>=3.12

Claimed: dincli/cli/dintoken.py:76 uses PEP 701 nested same-quote f-string syntax, a hard SyntaxError before 3.12, and it's the only offending file under dincli/.

Verified — real interpreter, not reasoning-from-spec: pulled a real cpython-3.11.15 build via uv run --python 3.11.15 and parsed the file directly. Got the exact error quoted in the PR:

SyntaxError: f-string: unmatched '(' (line 76)

Then swept every .py file under dincli/ through the same 3.11.15 parser — dintoken.py is the only one that fails. Matches the claim exactly, and unlike a same-Python-version reasoning check, this is a real cross-version parse, not an assumption about what PEP 701 does.

2. Four undeclared dependencies (rich, requests, eth-account, py-multibase)

Claimed: all four are imported directly in dincli/ but arrive only transitively via typer/web3/py-cid; at typer>=0.9.0's literal floor, a resolver can legitimately pick a typer release that doesn't pull in rich, and does — that's an install-breaking bug today, not hypothetical.

Verified — reproduced the actual crash, not just the version-string mismatch: built an isolated venv pinning typer==0.9.0 against unmodified develop's pre-PR dependency list (typer>=0.9.0, python-dotenv, web3, platformdirs, py-cid). import dincli.main in that venv:

ModuleNotFoundError: No module named 'rich'

at dincli/cli/aggregator.py:5. Confirmed rich is genuinely a base-only dep of newer typer but an extra == "all" dep at typer==0.9.0 — so this isn't a contrived pin, it's what the declared floor actually permits a resolver to choose.

Also confirmed each of the four is a real, direct (not just transitive-only) import: eth_account at cli/system.py:15 and cli/utils.py:13; requests at services/ipfs.py:8; rich across 10 files; multibase as a local/lazy import inside services/cid_utils.py:37 (get_cid_from_bytes32, used to re-encode a stored bytes32 digest back into a CIDv1 base32 string) — undeclared but real, since ast.walk in the PR's own dependency-audit test correctly reaches function-body imports too, not just top-level ones.

Correction claim also checked: grepped develop for any click import — none exists (main's need for it comes from cli/core.py, which doesn't exist on develop). Leaving click out of this PR is correct.

3. configure-ipfs added to the skip-list

Claimed: configure-ipfs's body never touches the wallet, so it shouldn't have been demanding one.

Verified by reading the actual function (cli/system.py, pre-PR): body only calls load_config(), resolve_ipfs_config(), normalize_ipfs_provider(), and ctx.obj.console — no get_en_w3_account_console(), no w3/account reference anywhere in it. So it was genuinely falling through to wallet setup it never used. Also confirmed the PR's narrower claim: "read-wallet" and "show-index" (the two other entries c5ed47c on main was said to fix) are already spelled correctly on develop — only configure-ipfs was actually missing.

4. read-din-per-eth

Claimed: dinPerEth was previously reachable only through the raw ABI JSON, with no CLI command to read it; this PR adds one shared helper in dintoken.py rather than duplicating it per role (the main approach).

Verified: grepped the whole repo for dinPerEth pre-PR — every hit outside Solidity/Hardhat sources is in Documentation//Developer/ design docs or dincli/abis/DinCoordinator.json; nothing in dincli/cli/. Confirmed the new command is registered on all three dintoken sub-apps (dintoken, aggregator dintoken, auditor dintoken) and that dinPerEth() is called from exactly one place in the whole tree (dincli/cli/dintoken.py) — genuinely shared, not duplicated the way main's version is.

Tests

Ran the new tests/test_develop_small_backports.py directly: 16/16 passing, including the two "guard" tests the PR calls out — flipped "read-wallet" to "read_wallet" by hand to confirm test_skip_list_entries_match_registered_command_names actually catches it (it does), and the dependency-audit test does walk function-body imports, not just top-level ones (confirmed via the multibase case above).

Full suite (tests/, excluding tests/dincli/ — an unrelated Hardhat-gated integration suite): 114 → 130, matching the claim exactly, measured on this branch against its own base.

One thing worth flagging, not a defect in this PR: this branch's merge-base is bf162c0, which predates #96 (already merged into develop, adding 47 tests of its own: 114 → 161 on develop proper). So the "114 → 130" delta is against this PR's own stale base, not current develop — after a real merge here I'd expect something like 161 → 177, not 114 → 130. Not a problem: the two PRs touch disjoint files (aggregator.py/auditor.py/dintoken.py/system.py/pyproject.toml here vs. ipfs.py/cid_utils.py/docs there), so this should merge cleanly with no reconciliation needed — just flagging it so the number isn't read against the wrong baseline.

Recommendation

Approve and merge. All four fixes reproduced under real conditions rather than taken on the PR's word — a real 3.11.15 parse for the syntax claim, a real dependency-resolution crash for the rich claim, direct reads of configure_ipfs's body and the skip-list, and a full-repo grep for dinPerEth's prior absence from the CLI. The self-correction (dropping click, which belongs in "PR B" per the linked discussion) also checks out — nothing on develop imports it today.

@umermjd11

Copy link
Copy Markdown
Collaborator

Actual outcome — PR #97 merged into develop and pushed

This supersedes the pre-merge review above with what actually happened merging this against develop.

One commit: a07fa34 — real merge (--no-ff, not squash) into develop. No conflicts, no deviation. All 6 files landed exactly as authored: dincli/cli/aggregator.py, dincli/cli/auditor.py, dincli/cli/dintoken.py, dincli/cli/system.py, pyproject.toml, tests/test_develop_small_backports.py.

One local wrinkle worth noting for the record, not a defect in this PR: this working tree already had an uncommitted, unrelated local edit to dincli/cli/system.py that happened to add the identical "configure-ipfs" skip-list entry this PR also adds. Stashed it before merging, merged clean, popped it back after — it now shows zero diff against the merged file, since both landed on the same fix independently. Nothing to reconcile.

Verification (post-merge, on the actual merged tree)

Foundry (develop's primary toolchain) — unaffected, confirmed rather than assumed: this PR touches no Solidity. forge build: 0 errors. forge test: all 16 suites, 162/162 passing, 0 failed.

Python (tests/, excluding tests/dincli/ — the unrelated Hardhat-gated integration suite, same scope as the #96 merge): 177/177 passing. Matches the number flagged in the pre-merge review exactly — the PR's own "114 → 130" was measured against its stale pre-#96 base; merged onto current develop (161 after #96) the real delta is 161 → 177, confirmed now rather than just predicted.

Recommendation

Nothing further needed here — every fix (Python floor, the four dependency declarations, the skip-list entry, read-din-per-eth) is now on develop, verified against the real merged tree, not just the isolated branch.

umermjd11 pushed a commit that referenced this pull request Aug 20, 2026
…evelop

Real merge conflict on pyproject.toml's dependencies list, predicted by the
PR itself ("Adds click to pyproject.toml; #97 touches the same list and was
opened first... needs a rebase with a one-line union"). Resolved by unioning
both sides: click (this PR) alongside rich/requests/eth-account (#97,
already merged). No other conflicts.

Adds RPC chain-id validation to get_w3(): a wrong-chain endpoint previously
surfaced only as "ETH Balance: 0", indistinguishable from an unfunded
wallet. Also fixes a pre-existing credential leak (the old handler put the
raw rpc_url and the provider's exception text into the raised error and its
traceback).
umermjd11 pushed a commit that referenced this pull request Aug 20, 2026
Real merge conflict on dincli/cli/system.py's skip-list, predicted by the
PR itself ("'bridge-eth' in the skip-list touches the same line as #97...
whichever merges second needs the union of both entries"). Resolved by
unioning both entries: configure-ipfs (#97, already merged) alongside
bridge-eth (this PR). No other conflicts (dincli/services/bridge.py and
tests/test_bridge_eth.py are both new files, no overlap).

Ports `dincli system bridge-eth` (L1 Sepolia -> L2 OP Sepolia ETH deposit)
from main. Commit 1 of the PR is a byte-identical port, verified via git
blob hash match against main's dincli/services/bridge.py. Commit 2 is a
self-caught follow-up: gas estimation on an unfunded L1 account raised an
uncaught Web3RPCError (raw traceback) instead of the readable
"insufficient L1 funds" rejection -- confirmed main has the identical
defect (no try/except around estimate_gas there either).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants