feat(api): native /api/v1/wallet lifecycle + key-derivation surface - #113
Conversation
|
Warning Review limit reached
More reviews will be available in 47 minutes and 46 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds native wallet lifecycle, address derivation, change-address management, and rescan endpoints to ChangesNative Wallet API Endpoints
ergo-node Wallet Bridge Error Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Second phase of the native wallet API (stacked on the read surface). Adds the
write side of lifecycle + keys and the centralized strict-JSON body extractor,
all over the EXISTING WalletAdmin trait methods (no new bridge commands).
Endpoints (api-key gated; bodies via the strict extractor):
- POST init (mnemonic returned once, no-store; strength validated 12/15/18/21/24),
POST restore (explicit DerivationMode required), POST unlock, POST lock,
POST mnemonic/verify (no-store; 409 on an uninitialized wallet).
- POST /addresses (derive: tagged next|path), GET/PUT /change-address
(PUT not unlock-gated; change address is public metadata, shown while locked),
POST rescan (real full-rebuild; rescan_unavailable on a non-replay backend).
StrictJson<T> extractor: deny_unknown_fields -> the native {reason:"bad_request"}
envelope (never Axum's default 400); empty body treated as `{}`; no-store helper
for secret-bearing responses.
Bridge correctness fixes (shared with the compat surface, strictly more correct;
no compat test depended on the old codes):
- init/restore now REFUSE to overwrite an existing wallet (WalletExists) instead
of persisting a second secret file -- a real data-loss guard.
- derive: bad path -> bad_request(400), duplicate path -> derivation_path_exists(409)
(were 500); rescan unsupported/in-progress -> rescan_unavailable(409) (was 500).
The tagged request enums (DeriveKeyRequest/DerivationMode) use manual Deserialize
so unknown sibling fields are rejected and per-variant fields validated (serde
can't deny_unknown_fields an internally-tagged enum).
Design + codex review (P2) in dev-docs (gitignored); every finding folded in
(init/restore overwrite guard, mnemonic-verify-uninitialized 409, derive error
classification, rescan 409s, fallible address index, tagged-enum strictness).
Test plan: cargo fmt --all -- --check; cargo clippy --workspace --all-targets
--all-features -- -D warnings; cargo test --workspace (259 test binaries, 0
failures). New: init-twice -> WalletExists, strict-JSON native envelope,
init-bad-strength, bodyless-rescan, POST gating, tagged-enum strictness, DTO shapes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cd4fab5 to
aa4be13
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ergo-node/tests/wallet_admin_roundtrip.rs (1)
575-611: ⚡ Quick winAdd a symmetric
restoreoverwrite regression test.This test locks in the new
initguard well; adding arestore-twice case would similarly protect theWalletExistscontract added forrestore.Proposed test shape
+#[tokio::test] +async fn restore_twice_returns_wallet_exists() { + // setup identical to init_twice_returns_wallet_exists + // first restore succeeds + // second restore expect_err and assert matches!(err, WalletAdminError::WalletExists) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ergo-node/tests/wallet_admin_roundtrip.rs` around lines 575 - 611, Create a new async test function named restore_twice_returns_wallet_exists that mirrors the structure of init_twice_returns_wallet_exists. Set up identical test infrastructure with temporary directory, storage, state, database, and wallet admin components. Instead of calling admin.init() twice, call admin.restore() twice with appropriate parameters and verify that the second restore call returns a WalletAdminError::WalletExists error using the same assertion pattern. This protects the WalletExists contract for the restore function similar to how the existing test_init_twice_returns_wallet_exists guards the init function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ergo-api/src/wallet/native/mod.rs`:
- Around line 576-579: The utoipa::path annotation for the POST
/api/v1/wallet/rescan endpoint currently marks the request body
(dto::RescanRequest) as required in the OpenAPI spec, but the handler
implementation allows it to be optional. Modify the utoipa::path annotation by
adding the appropriate parameter to mark the request_body as optional (using the
required property or wrapper). After making this change to the annotation,
regenerate the OpenAPI fixture to reflect the updated contract where the
RescanRequest body is optional.
---
Nitpick comments:
In `@ergo-node/tests/wallet_admin_roundtrip.rs`:
- Around line 575-611: Create a new async test function named
restore_twice_returns_wallet_exists that mirrors the structure of
init_twice_returns_wallet_exists. Set up identical test infrastructure with
temporary directory, storage, state, database, and wallet admin components.
Instead of calling admin.init() twice, call admin.restore() twice with
appropriate parameters and verify that the second restore call returns a
WalletAdminError::WalletExists error using the same assertion pattern. This
protects the WalletExists contract for the restore function similar to how the
existing test_init_twice_returns_wallet_exists guards the init function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7282edef-2c53-40af-a580-5f63150d42ca
📒 Files selected for processing (8)
ergo-api/src/server.rsergo-api/src/wallet/native/dto.rsergo-api/src/wallet/native/mod.rsergo-api/tests/fixtures/openapi_native.yamlergo-api/tests/openapi_native_runtime_mount.rsergo-node/src/node/wallet_bridge.rsergo-node/src/node/wallet_bridge/commands/admin.rsergo-node/tests/wallet_admin_roundtrip.rs
CodeRabbit (PR #113): the strict extractor treats an empty body as `{}` (a bodyless rescan POST does a full rebuild, covered by the runtime-mount test), but the utoipa annotation generated `requestBody.required: true` — a contract mismatch that would make generated SDKs require the body. Use `request_body = Option<dto::RescanRequest>` so the spec marks it optional; golden regenerated. Gate: fmt + clippy --workspace --all-targets --all-features -D warnings clean; cargo test --workspace green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lease cut (#126) 0.4.3 was version-cut at #117, but #106–#114 and #118–#125 landed at the same version without a CHANGELOG entry — so [0.4.3] documented only #115/#116/#117 while the deployed 0.4.3 binary actually contains all 18 PRs since the v0.4.2 cut. Backfills the section to match what shipped: - New ### Added subsection for the native /api/v1/wallet surface (#112, #113, #114). - ### Fixed now covers the signed-byte parity fixes (#106, #107), the Coll-equality cost short-circuit (#108), EIP-27 re-emission enforcement (#109 + #111, merged — the P0 fork fix, now across block/mempool/mining), and the full v6/EIP-50 + ErgoTree wire-deserialization cluster (#118–#125). - Broadened the release intro to name the EIP-27 enforcement and the native wallet surface alongside the v6/EIP-50 conformance work. - Added the missing (#117) reference to the box-deserialize entry. Docs only; no code or behavior change. Entries were drafted from each PR's own commit/description and accuracy-reviewed (e.g. the EIP-27 soak figure is the corrected 172 reward-box burns, not the repudiated 1,173). Co-authored-by: arkadianet <rkadias@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
What & why
Second phase of the native
/api/v1/wallet/*API — the lifecycle & key-derivation (write) surface — stacked on #112 (the read surface). All nine endpoints are thin adapters over the existingWalletAdmintrait methods, so there are no new bridge commands; the headline new infrastructure is the centralized strict-JSON body extractor that the write surface needs.Endpoints (api-key gated; strict bodies)
/init/restoreDerivationModerequired/unlock/lock/mnemonic/verify{matched};409on an uninitialized wallet/addressesnext|path)/change-address/rescanrescan_unavailableon a non-replay backendKey points
StrictJson<T>extractor:deny_unknown_fields→ the native{reason:"bad_request"}envelope (never Axum's default 400); empty body treated as{}; ano_storehelper for secret-bearing responses (initmnemonic,mnemonic/verify).init/restorenow refuse to overwrite an existing wallet (WalletExists) instead of writing a second secret file — a real data-loss guard; derive bad-path →400, duplicate →409; rescan unsupported/in-progress →409(all were500).DeriveKeyRequest/DerivationMode) use manualDeserializeso unknown sibling fields are rejected and per-variant fields validated (serde can'tdeny_unknown_fieldsan internally-tagged enum).Review process
Design + a dedicated codex review of P2 in
dev-docs/(gitignored). Every finding folded in — most importantly theinit/restoreoverwrite guard codex caught, plusmnemonic/verify-uninitialized → 409, derive error classification, rescan 409s, the fallible address index, and the tagged-enum strictness.Test plan
cargo fmt --all -- --check— cleancargo clippy --workspace --all-targets --all-features -- -D warnings— cleancargo test --workspace— 259 test binaries, 0 failuresNew tests: init-twice →
WalletExists, strict-JSON native envelope, init bad-strength, bodyless rescan, POST gating, tagged-enum strictness, all new DTO shapes.Next
P3 — transaction construction (build/sign/send + the burn-aware builder), the second half of closing the original invalid-tx bug. Its own gated, codex-reviewed PR.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests