feat(sdk): address funding fee quote client - #4446
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
✅ Final review complete — no blockers (commit a578ef1) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The transport, response parsing, query encoding, and protocol-version guard are consistent with the existing unproved-query architecture, and no blocking defects were identified. Three non-blocking gaps remain in mock coverage and public API completeness.
Source: codex-general and codex-rust-quality reviewer evidence (exact Codex backend model IDs were not supplied); final verifier backend model: grok-4.5; orchestration-only and not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 3 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk/src/mock/requests.rs`:
- [SUGGESTION] packages/rs-sdk/src/mock/requests.rs:506: MockResponse support is not connected to FetchUnproved
The new serialization implementation satisfies the `MockResponse` bound, but the unproved-fetch path never consults the typed expectation cache. `MockDashPlatformSdk::expect_fetch` only accepts `Fetch`, and `load_expectations_sync` has no `GetAddressFundingFeeQuoteRequest` arm, so callers cannot configure `Sdk::new_mock()` to return this quote. Add a typed unproved-response expectation path or endpoint-specific raw protobuf expectation support, then exercise `quote_address_funding_fee` and its unknown-protocol-version guard through the mock SDK.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/quote.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/quote.rs:36: Wallet quotes always discard a known signable-length hint
The server uses a calibrated 390-byte default but explicitly supports the real signable length for larger funding transactions. This wrapper always sends `None`, even though its documented `prepared_outpoint` case represents an already-built and signed lock whose caller may know the future transition size. Expose an optional `signable_bytes_len_hint` parameter, or provide a wallet method accepting a fully specified `AddressFundingFeeQuoteQuery`, so callers can request the more accurate hashing charge.
In `packages/rs-sdk/src/platform/address_funding_fee_quote.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/address_funding_fee_quote.rs:29-34: Keep asset-lock outpoints typed until wire encoding
The public query exposes an asset-lock outpoint as `[u8; 36]`, making callers responsible for txid byte order and little-endian vout encoding. `dpp::dashcore::OutPoint` is the existing domain type and has the canonical conversion to the 36-byte representation used by Platform; the wallet's asset-lock flows already use it. Accept `Option<OutPoint>` in the SDK query and wallet wrapper, and convert it to bytes only while constructing the protobuf request.
| impl_mock_response!(ElementFetchRequestItem); | ||
| impl_mock_response!(EvoNodeStatus); | ||
| impl_mock_response!(CurrentQuorumsInfo); | ||
| impl_mock_response!(AddressFundingFeeQuote); |
There was a problem hiding this comment.
🟡 Suggestion: MockResponse support is not connected to FetchUnproved
The new serialization implementation satisfies the MockResponse bound, but the unproved-fetch path never consults the typed expectation cache. MockDashPlatformSdk::expect_fetch only accepts Fetch, and load_expectations_sync has no GetAddressFundingFeeQuoteRequest arm, so callers cannot configure Sdk::new_mock() to return this quote. Add a typed unproved-response expectation path or endpoint-specific raw protobuf expectation support, then exercise quote_address_funding_fee and its unknown-protocol-version guard through the mock SDK.
source: ['codex']
| recipient, | ||
| asset_lock_outpoint: prepared_outpoint, | ||
| user_fee_increase, | ||
| signable_bytes_len_hint: None, |
There was a problem hiding this comment.
🟡 Suggestion: Wallet quotes always discard a known signable-length hint
The server uses a calibrated 390-byte default but explicitly supports the real signable length for larger funding transactions. This wrapper always sends None, even though its documented prepared_outpoint case represents an already-built and signed lock whose caller may know the future transition size. Expose an optional signable_bytes_len_hint parameter, or provide a wallet method accepting a fully specified AddressFundingFeeQuoteQuery, so callers can request the more accurate hashing charge.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Wallet quotes always discard a known signable-length hint no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| /// The exact planned asset lock outpoint (txid bytes followed by the | ||
| /// vout as four little-endian bytes), when the wallet has already built | ||
| /// and signed the lock transaction. `None` lets the node derive a | ||
| /// deterministic placeholder — for a fresh (absent) outpoint both have | ||
| /// the same expected search depth. | ||
| pub asset_lock_outpoint: Option<[u8; 36]>, |
There was a problem hiding this comment.
🟡 Suggestion: Keep asset-lock outpoints typed until wire encoding
The public query exposes an asset-lock outpoint as [u8; 36], making callers responsible for txid byte order and little-endian vout encoding. dpp::dashcore::OutPoint is the existing domain type and has the canonical conversion to the 36-byte representation used by Platform; the wallet's asset-lock flows already use it. Accept Option<OutPoint> in the SDK query and wallet wrapper, and convert it to bytes only while constructing the protobuf request.
source: ['codex']
faa108a to
fbc9836
Compare
86be968 to
08d325a
Compare
571a00d to
7e37071
Compare
08d325a to
06ee2ac
Compare
Client plumbing for getAddressFundingFeeQuote, end to end through the idiomatic unproved-query path: - rs-dapi-client: transport impl for the new request (standard impl_transport_request_grpc! block); - rs-drive-proof-verifier: AddressFundingFeeQuote type and its FromUnproved impl (the response is a computed value with metadata and deliberately no proof — the type docs say to treat it as planning data); - rs-sdk: FetchUnproved wiring, mock-response support, and a new platform::address_funding_fee_quote module with AddressFundingFeeQuoteQuery (recipient, optional exact 36-byte outpoint — None lets the node use its deterministic placeholder — user_fee_increase, optional signable-bytes length hint) plus quote_address_funding_fee(sdk, query), which fails fast with a protocol error when the node quoted with a protocol version this client does not know; - rs-platform-wallet: PlatformAddressWallet::quote_funding_fee — a thin wrapper over the SDK call using the wallet's own SDK handle, with no offline fallback: a network failure surfaces as an error, never a silently stale constant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
06ee2ac to
a578ef1
Compare
7e37071 to
14fdd09
Compare
Issue being fixed or feature implemented
Third PR of the address-funding fee-quote stack (3/4, on top of #4445). Client plumbing for
getAddressFundingFeeQuote, end to end through the idiomatic unproved-query path.What was done?
impl_transport_request_grpc!block).AddressFundingFeeQuotetype and itsFromUnprovedimpl — the response is a computed value with metadata and deliberately no proof; the type docs say to treat it as planning data.FetchUnprovedwiring, mock-response support, and a newplatform::address_funding_fee_quotemodule withAddressFundingFeeQuoteQuery(recipient, optional exact 36-byte outpoint —Nonelets the node use its deterministic placeholder —user_fee_increase, optional signable-bytes length hint) plusquote_address_funding_fee(sdk, query), which fails fast with a protocol error when the node quoted with a protocol version this client does not know.PlatformAddressWallet::quote_funding_fee— a thin wrapper over the SDK call using the wallet's own SDK handle, with no offline fallback: a network failure surfaces as an error, never a silently stale constant.How Has This Been Tested?
cargo check/clippyon dash-sdk, platform-wallet, rs-dapi-client, drive-proof-verifier); quote-vs-execution behaviour is covered by the server-side tests in feat(platform): getAddressFundingFeeQuote query #4445.Breaking Changes
None — additive.
Checklist:
🤖 Generated with Claude Code