Skip to content

feat: Sapphire testnet as HONOR deposit source chain - #246

Open
rube-de wants to merge 10 commits into
masterfrom
rube/feature/sapphire-token-support
Open

feat: Sapphire testnet as HONOR deposit source chain#246
rube-de wants to merge 10 commits into
masterfrom
rube/feature/sapphire-token-support

Conversation

@rube-de

@rube-de rube-de commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

closes #245

Register Sapphire testnet (23295) as an ERC-20 deposit source chain for HONOR. The accounting chain doubles as its own deposit source: chain config, RPC wiring, discovery, sweeps, credits, and withdrawals all on one chain.

What changed beyond config

  • contract: gasLimitNativeSweep 21,000 -> 25,000 (measured native transfer on Sapphire: 22,140 gas; the old limit failed every native sweep and the gas-funding leg of every ERC-20 sweep). VERSION -> 2 so the upgrade task acts.
  • RPC identity gate: every configured endpoint must report its filed chain ID at startup, re-verified every 5 minutes. A mismatch leaves the chain unserved, never mis-served. All chain-facing consumers route through the gate, including the withdrawal broadcaster.
  • withdrawal safety: duplicate-broadcast proof requires a status-1 receipt for the exact signed bytes, so reverted receipts and mempool entries never mark a withdrawal paid. A per-chain nonce gate refuses to sign into spent nonces, admission thresholds come from the contract's own gas limits, and catch-up targets derive from the latest nonce so a stuck resolved withdrawal is recoverable (stuck-nonce alarm included).
  • /deposits/address lists minimums only for verified chains with registered assets. 23295 advertises HONOR only, no native ROSE.

Gas funding is sized from the contract's sweep gas limit at the signing price plus 30% headroom; the static per-chain amounts stay as fallback when the contract read fails. Known tradeoff: the sweep is signed at the pre-funding gas price, so a spike during funding gives a slow sweep, not an out-of-gas one.

The rubetest shadow-deployment harness stays in the repo for re-rehearsing the rollout. The localnet mirror (23293) only exists when SAPPHIRE_CHAIN_ID selects it.

Verification

  • full CI gate list green locally: format, lint, typecheck, solidity build, bytecode size budget, validate-upgrade, ABI presence, OpenAPI drift, pytest (716 passed), hardhat (98 in-process + 60 against sapphire-localnet)
  • live rehearsal on an isolated testnet app and proxy: boot checks (3-chain identity, signer publish, HONOR registration, gas price publish), deposit -> gas funding -> sweep -> credit, withdrawal, machine-restart sweep recovery
  • permissionless front-run of a withdrawal: the exact-hash check resolved it correctly
  • V1 -> V2 upgrade rehearsed on a live shadow proxy: state preserved, version gate skips on re-run

Deploy

A startup version gate refuses to run against an implementation below REQUIRED_ACCOUNTING_VERSION (2), so token registration cannot precede the proxy upgrade. After merge:

  • hardhat upgrade --network sapphire-testnet (add --output-safe if the owner is a Safe)
  • tag privana/v1.0.3-testnet
  • update the compose digest, redeploy ROFL

PR REVIEW

Register chain 23295 end to end for ERC-20 HONOR deposits, discovery,
sweeps, credits, and withdrawals:

- seed SAPPHIRE_RPC_URL into chain_rpc_urls independently of Alchemy
- add 23295 (and 23293 localnet mirror) chain configs with the 100-block
  eth_getLogs cap and gas funding sized from gas limit x gas price
- fail-closed startup RPC identity check: endpoints must report the
  chain id they are filed under or the chain goes unserved
- advertise deposit minimums per registered asset type; 23295 offers
  HONOR only, no native ROSE
- withdrawal safety: destination-nonce readiness gate, exact-hash
  duplicate-broadcast verification replacing error-string matching,
  and gas-derived withdrawal admission from contract gas limits
- honest validate-upgrade messaging (implementation-level checks only)
- same-chain E2E test and deterministic localnet ERC-20 harness
- drop dead SWEEP_GAS_LIMIT_*/GAS_FUNDING_GAS_LIMIT constants
Sapphire native transfers cost 22,140 gas (measured from receipts;
Base Sepolia baseline 21,000). The old 21,000 limit blocked native
sweeps and the gas-funding leg of every ERC-20 sweep on 23295.
Unused gas is refunded, so existing chains are unaffected.

VERSION bump to 2 is required for the upgrade task to act
(deploy.ts skips when currentVersion >= availableVersion).

Tests assert the constant, the encoded gas limit in generated sweep
and gas-funding transactions (via Sapphire signed queries), and
proxy state preservation across the upgrade.
Comment-only pass over the Sapphire source-chain change set:

- state rationale once at the definition site; call sites point back
- drop step narration and assert-restating labels in tests
- replace plan-document milestone tags with self-contained wording
- fix stale claims (typed-tx hash wording, hard line-number citations)
- record the measured 22,140-gas floor at gasLimitNativeSweep itself
The shadow ROFL rehearsal (full HONOR deposit/sweep/credit/withdraw on
a dedicated testnet app) is retained as a repeatable dev harness:

- rofl.yaml: rubetest deployment (app rofl1qz20hu6qztr665w0t4d6y75fys6nq0j3xg24y0ey,
  admin: easy, playground machine, compose.rubetest.yaml)
- compose.rubetest.yaml: digest-pinned shadow image, no custom domain
- .env.rubetest: baked env pointing at shadow proxy 0x34D9E2...64B6b

Re-running costs one machine deploy + signer funding; the app id and
OCI bundle stay registered.
Comment-only pass over the test changes on this branch: delete
assert-restating and step-narrating comments, tighten the ones that
carry non-obvious context (mock semantics, Sapphire quirks, invariants).
Master-era comments are untouched.
- duplicate-broadcast proof now requires a status-1 receipt for the
  exact signed bytes; reverted receipts and mempool entries no longer
  mark a withdrawal paid
- AccountingContractService reads the verified RPC set live and routes
  destination-chain clients through the identity gate (was the one
  fund-moving consumer built pre-check with a stale snapshot)
- /deposits/address advertises min_deposit only for verified chains
  with registered assets; empty chain entries are omitted
- withdrawal gas limits cached with a 5-minute TTL like the sibling
  contract caches
- lifespan refuses to start against an implementation below
  REQUIRED_ACCOUNTING_VERSION before registering tokens
- branch-added solidity tests derive extra signers via getDeployer so
  a single-key SECRET_KEY config no longer TypeErrors
- 23293 added to chain metadata dicts
…ardening

- gas funding sized from the contract's sweep gas limit at the price
  the sweep is signed with (plus 30% headroom); static per-chain
  amounts remain as the read-failure fallback
- catch-up re-broadcast targets derived from the latest nonce so an
  unmined resolved withdrawal is recoverable; stuck-nonce alarm after
  10 cycles; last successful broadcast hash remembered per withdrawal
  so a gas-price republish cannot hide our own mined payout
- RPC identity gate fails closed before the check runs (explicit
  test-only opt-out) and re-verifies every 5 minutes, readmitting
  recovered endpoints and dropping drifted ones
- 23293 chain config present only when SAPPHIRE_CHAIN_ID is the
  localnet id; localnet faucet mint restricted to the deployer
- sapphire discovery lookback capped at 1,000 blocks; rubetest
  deployment: INFO logging and a log.view permission list
@rube-de
rube-de marked this pull request as ready for review August 19, 2026 17:24
Copilot AI lite review requested due to automatic review settings August 19, 2026 17:24

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Registers Sapphire testnet (23295) as an HONOR ERC-20 deposit source chain and supports “same-chain” operation where the accounting chain is also the deposit source, with additional safety hardening around RPC endpoint identity, gas-funding sizing, and withdrawal broadcast correctness.

Changes:

  • Add an RPC identity gate (rpc_identity) that probes eth_chainId at startup and periodically re-verifies endpoints, wiring all chain-facing services through verified_web3.
  • Improve sweep gas funding by sizing from contract sweep gas-limit constants (with headroom) and aligning sweep signing gas price with the funding leg.
  • Harden withdrawal broadcasting by proving duplicates via status-1 receipts for exact signed payload hashes, adding nonce readiness gating, and adding stuck-nonce alarming; expand test coverage accordingly.

Reviewed changes

Copilot reviewed 37 out of 37 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
test/py/test_withdrawals.py Adds extensive tests for withdrawal duplicate-broadcast proof, nonce gating, and admission logic.
test/py/test_sweep_engine.py Updates tests for contract-derived gas funding sizing and fallback behavior.
test/py/test_same_chain_e2e.py Adds E2E coverage for same-chain deposit→sweep→credit and same-chain withdrawals under verified RPC gating.
test/py/test_rpc_identity.py Adds unit tests for RPC identity verification, fail-closed behavior, and re-verification loop behavior.
test/py/test_main_lifespan.py Adds lifespan-ordering tests ensuring RPC identity runs before any chain-touching bootstrap and that drifted/mis-filed chains are excluded.
test/py/test_chain_config.py Adds Sapphire chain config tests, localnet gating tests, and gas-funding sanity checks.
test/py/test_accounting_routes.py Updates /deposits/address behavior to advertise minimums only for verified chains with registered asset types; adds tests.
test/py/test_accounting_contract_service.py Adds tests for identity-gated chain reads and TTL-cached contract gas limit getters.
test/conftest.py Resets global RPC identity state between tests and enables pre-check fallback for unit tests.
src/services/withdrawal_processor.py Implements nonce readiness gate, duplicate broadcast proof via receipts, “remembered hash” logic, and stuck-nonce alarming; uses verified RPC clients.
src/services/sweep_engine.py Switches to verified RPC clients and sizes gas-funding from contract gas-limit constants with headroom; keeps sweep signing price aligned with funding.
src/services/rpc_identity.py Introduces the RPC chain-id verification gate, commit/narrowing behavior, and periodic re-verification loop.
src/services/deposit_verifier.py Switches source-chain Web3 construction to the RPC identity gate (verified_web3).
src/services/deposit_discovery.py Switches discovery Web3 construction to the RPC identity gate and updates error semantics.
src/services/accounting_contract.py Switches chain Web3 resolution to verified_web3; adds TTL-cached gas-limit getters; improves withdrawal gas admission sizing.
src/README.md Updates documentation wording for DepositAccountingProtocol mocking guidance.
src/models/accounting.py Improves OpenAPI schema description for min_deposit.
src/main.py Runs RPC identity verification first in lifespan; starts/stops re-verification loop; adds accounting VERSION gate.
src/config/chain_config.py Adds Sapphire testnet config and conditional Sapphire localnet config based on SAPPHIRE_CHAIN_ID.
src/config/init.py Seeds Sapphire RPC URL independently of Alchemy; returns Sapphire mapping even when Alchemy key is unset.
src/api/routes.py Adjusts /deposits/address response to only include verified+registered asset minimums and finality depths for served chains.
solidity/test/EVMSignerAndVerifier.ts Adds tests asserting native sweep gas limit is 25k and encoded into generated txs on Sapphire-like networks.
solidity/test/Accounting.E2E.ts Extends upgrade tests to assert VERSION==2 and preserve state across upgrade flows.
solidity/tasks/localnetToken.ts Adds Hardhat task to deploy deterministic localnet ERC-20 (LHONOR) for same-chain dev harness.
solidity/tasks/index.ts Registers the new deploy-localnet-token task.
solidity/tasks/deploy.ts Hooks localnet token deployment into deploy flow when on Sapphire localnet.
solidity/scripts/validate-upgrade.ts Clarifies upgrade validation logging and what is/isn’t being validated.
solidity/contracts/test/LocalnetERC20.sol Adds localnet-only ERC-20 contract used by same-chain localnet harness.
solidity/contracts/EVMSignerAndVerifier.sol Bumps gasLimitNativeSweep from 21,000 to 25,000 and updates sweep semantics comment.
solidity/contracts/Accounting.sol Bumps contract VERSION from 1 to 2.
rofl.yaml Adds a rubetest deployment profile and associated configuration.
docs/openapi.json Updates OpenAPI to include min_deposit field description.
compose.rubetest.yaml Adds a compose file for the rubetest shadow-deployment harness.
.gitleaksignore Adds allowlist entries for known false positives (addresses/test key) introduced by new files.
.env.testnet Adds chain 23295 gas price and HONOR token registration entry.
.env.rubetest Adds a full rubetest environment configuration including Sapphire chain and HONOR token info.
.env.localnet Adds Sapphire localnet gas price and deterministic LHONOR token registration entry.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/services/deposit_verifier.py Outdated
Comment thread src/services/deposit_discovery.py Outdated
Comment thread src/services/sweep_engine.py Outdated
Comment thread src/services/withdrawal_processor.py Outdated
Comment thread src/services/accounting_contract.py Outdated
- fold _remembered_broadcast_tx into _find_broadcast_tx as an ordered
  candidate lookup (remembered hash first); same status-1-only proof,
  mempool entries still unproven
- return (contract_next, chain_latest) from _chain_nonce_state; the
  pending-nonce gate and stuck-nonce tracking stay internal
- skip token-context resolution for non-target nonces in catch-up scan
- share one require_verified_web3 helper across the four per-service
  memoized verified_web3 lookups
- drop dead CancelledError re-raise and redundant dict copies in
  rpc_identity
- derive the 23293 chain config from the 23295 entry via
  dataclasses.replace
- remove dead SAPPHIRE_CHAIN_ID env fallback from _build_chain_rpc_urls
require_verified_web3 and deposit_discovery._get_web3 re-consult the
verified set on every call instead of short-circuiting on a cache hit,
so a chain dropped by the re-verification loop stops resolving
immediately. The per-instance cache now only pins client identity
while the chain stays served.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/services/rpc_identity.py:236

  • _reverification_loop() catches Exception, which also catches asyncio.CancelledError on modern Python. That can cause the background loop to ignore cancellation (e.g., during shutdown), log a stack trace, and continue running.

Re-raise asyncio.CancelledError explicitly before the generic handler so stop_reverification_loop() can reliably cancel and await the task.

async def _reverification_loop(settings, interval_seconds: float, timeout: float) -> None:
    while True:
        await asyncio.sleep(interval_seconds)
        try:
            await reverify_chain_rpc_urls(settings, timeout=timeout)
        except Exception:
            # Keep the loop alive: the current verified set stays as it is, and the
            # next pass gets another chance to drop or readmit.
            logger.exception("RPC identity re-verification pass failed; verified set unchanged")

src/services/withdrawal_processor.py:443

  • In the catch-up rebroadcast path, _find_broadcast_tx() returns None both when there is no mined receipt and when the expected tx is merely pending (it logs an info line and returns None). The error log here then asserts the nonce "may have been spent by a different transaction" / "leaving this withdrawal unpayable", which is misleading in the common case where the tx is still in flight.

Consider softening the message (or explicitly handling the pending-tx case) so operators don't get a false 'unpayable' signal.

                    logger.error(
                        f"Withdrawal #{index}: broadcast failed and no successful transaction "
                        f"matching the signed payload exists on {chain_name} - nonce {nonce} may "
                        f"have been spent by a different transaction, leaving this withdrawal "
                        f"unpayable: "

Comment on lines +101 to +113
for chain_id, reported in zip(chain_ids, reported_ids):
if isinstance(reported, BaseException):
if not isinstance(reported, Exception):
# CancelledError: the caller is shutting down, not a failed probe.
raise reported
logger.error(
"RPC identity check failed for chain %s (%s: %s) — endpoint excluded, "
"chain unserved until a later probe gets an answer",
chain_id,
type(reported).__name__,
reported,
)
continue

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/services/withdrawal_processor.py:308

  • _find_broadcast_tx() only checks the mempool for expected_hash. When resolveWithdrawal re-signs (e.g., after setGasPrice) and the previously broadcast tx is still pending under the remembered hash, this path won't detect it as in-flight, and callers will log a scary failure and pause processing even though the withdrawal may simply be awaiting mining. Consider checking eth_getTransactionByHash for all candidate hashes (remembered + expected).
        # Nothing mined. A mempool entry proves nothing either way, so this lookup only
        # reports, and today's hash is the one a retry would put back on the wire.
        dest_web3 = self._get_destination_web3(chain_id)

        async def fetch_transaction():
            try:
                return await dest_web3.eth.get_transaction(expected_hash)
            except TransactionNotFound:

src/services/withdrawal_processor.py:443

  • This error log claims the withdrawal may be “unpayable” due to a foreign-spent nonce, but in this code path _find_broadcast_tx() intentionally treats pending/mempool entries as "not yet proven" (no status-1 receipt). That means this message can be emitted even when our transaction is simply in flight, which is misleading and can cause unnecessary operator escalation. Suggest rewording to avoid implying irrecoverability when the state is just unproven.
                    logger.error(
                        f"Withdrawal #{index}: broadcast failed and no successful transaction "
                        f"matching the signed payload exists on {chain_name} - nonce {nonce} may "
                        f"have been spent by a different transaction, leaving this withdrawal "
                        f"unpayable: "
                        f"{exc}"
                    )

src/main.py:137

  • start_reverification_loop(settings) is started before several startup steps that can raise (e.g., key initialization, get_accounting_version() gate, token registration). If startup aborts before reaching yield, the shutdown path (which calls stop_reverification_loop()) never runs, potentially leaving a background task running in a partially-initialized process (notably in test harnesses or supervisor-managed processes that don't immediately exit). Consider starting the loop only after successful startup, or wrapping the startup sequence in a try/finally that stops the loop on exceptions before yield.
    # Endpoints drift while the service runs; re-probe on a timer so a drifted one
    # loses its chain and a recovered one gets it back without a restart.
    start_reverification_loop(settings)

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.

Add support for Sapphire tokens in Privana

2 participants