feat(platform): getAddressFundingFeeQuote query - #4445
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 |
|
⛔ Blockers found — Opus deferred (commit 14fdd09) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The query is integrated through the versioned DAPI and Drive paths and reuses the existing estimator, but it accepts non-canonical address encodings and can underquote both deliberately understated and valid large signable-length hints. The calibration test also does not enforce the fee-relevant default it claims to pin, so changes are required before relying on this endpoint for wallet fee planning. Source: reviewers openai/gpt-5.4 (codex-general, codex-rust-quality, codex-security-auditor); final verifier grok-4.5; orchestration-only openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 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-drive-abci/src/query/address_funds/address_funding_fee_quote/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/query/address_funds/address_funding_fee_quote/v0/mod.rs:62-68: Reject trailing bytes in serialized addresses
`PlatformAddress::from_bytes` uses `bincode::decode_from_slice` but discards the consumed-byte count, so these lines accept a canonical 21-byte address followed by arbitrary trailing bytes. That violates the RPC's documented 21-byte format and the PR's malformed-address rejection guarantee. When the outpoint is omitted, the handler also copies and hashes the entire non-canonical vector while constructing the placeholder. Require an exact canonical encoding before running the estimate.
- [BLOCKING] packages/rs-drive-abci/src/query/address_funds/address_funding_fee_quote/v0/mod.rs:31-33: Make signable-length bounds cover the valid fee range
The bounds permit both forms of fee understatement that the API says clamping prevents. A nonzero hint below the measured 390-byte default is reduced to 128 bytes, changing the production hash charge from six blocks to two, while every hint above 8,192 bytes is priced as 8,192. The current protocol accepts state transitions up to 20,480 bytes and permits asset-lock transactions with up to 100 inputs, so valid funding transitions can have signable payloads well above 8 KiB; for example, a 15 KiB payload is charged for about 240 blocks during `transform_into_action` but only 128 blocks by this quote. Set the lower bound to the conservative measured default and allow hints through the versioned `platform_version.system_limits.max_state_transition_size`; using 20,480 as the constant ceiling corrects the current protocol.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs:10400-10405: Tighten the signable-length calibration assertion
This factor-of-two interval does not provide the calibration pin described by the test and handler comments. The measured fixture could grow from 390 to 780 bytes, doubling the production hash-block count from six to twelve, while the test would continue passing and the default quote would remain unchanged. Compare the fee-relevant 64-byte block counts so harmless byte-level variation within one pricing block remains allowed but any change to the charged block count forces an update.
| let Ok(recipient) = PlatformAddress::from_bytes(&address) else { | ||
| return Ok(QueryValidationResult::new_with_error( | ||
| QueryError::InvalidArgument( | ||
| "address must be a serialized platform address".to_string(), | ||
| ), | ||
| )); | ||
| }; |
There was a problem hiding this comment.
🔴 Blocking: Reject trailing bytes in serialized addresses
PlatformAddress::from_bytes uses bincode::decode_from_slice but discards the consumed-byte count, so these lines accept a canonical 21-byte address followed by arbitrary trailing bytes. That violates the RPC's documented 21-byte format and the PR's malformed-address rejection guarantee. When the outpoint is omitted, the handler also copies and hashes the entire non-canonical vector while constructing the placeholder. Require an exact canonical encoding before running the estimate.
| let Ok(recipient) = PlatformAddress::from_bytes(&address) else { | |
| return Ok(QueryValidationResult::new_with_error( | |
| QueryError::InvalidArgument( | |
| "address must be a serialized platform address".to_string(), | |
| ), | |
| )); | |
| }; | |
| let Some(recipient) = PlatformAddress::from_bytes(&address) | |
| .ok() | |
| .filter(|recipient| recipient.to_bytes() == address) | |
| else { | |
| return Ok(QueryValidationResult::new_with_error( | |
| QueryError::InvalidArgument( | |
| "address must be a serialized platform address".to_string(), | |
| ), | |
| )); | |
| }; |
source: ['codex']
| pub(crate) const MIN_SIGNABLE_BYTES_LEN_HINT: u32 = 128; | ||
| pub(crate) const DEFAULT_SIGNABLE_BYTES_LEN_HINT: u32 = 390; | ||
| pub(crate) const MAX_SIGNABLE_BYTES_LEN_HINT: u32 = 8_192; |
There was a problem hiding this comment.
🔴 Blocking: Make signable-length bounds cover the valid fee range
The bounds permit both forms of fee understatement that the API says clamping prevents. A nonzero hint below the measured 390-byte default is reduced to 128 bytes, changing the production hash charge from six blocks to two, while every hint above 8,192 bytes is priced as 8,192. The current protocol accepts state transitions up to 20,480 bytes and permits asset-lock transactions with up to 100 inputs, so valid funding transitions can have signable payloads well above 8 KiB; for example, a 15 KiB payload is charged for about 240 blocks during transform_into_action but only 128 blocks by this quote. Set the lower bound to the conservative measured default and allow hints through the versioned platform_version.system_limits.max_state_transition_size; using 20,480 as the constant ceiling corrects the current protocol.
| pub(crate) const MIN_SIGNABLE_BYTES_LEN_HINT: u32 = 128; | |
| pub(crate) const DEFAULT_SIGNABLE_BYTES_LEN_HINT: u32 = 390; | |
| pub(crate) const MAX_SIGNABLE_BYTES_LEN_HINT: u32 = 8_192; | |
| pub(crate) const DEFAULT_SIGNABLE_BYTES_LEN_HINT: u32 = 390; | |
| pub(crate) const MIN_SIGNABLE_BYTES_LEN_HINT: u32 = DEFAULT_SIGNABLE_BYTES_LEN_HINT; | |
| pub(crate) const MAX_SIGNABLE_BYTES_LEN_HINT: u32 = 20_480; |
source: ['codex']
| assert!( | ||
| DEFAULT_SIGNABLE_BYTES_LEN_HINT >= measured / 2 | ||
| && DEFAULT_SIGNABLE_BYTES_LEN_HINT <= measured.saturating_mul(2), | ||
| "default hint {DEFAULT_SIGNABLE_BYTES_LEN_HINT} must stay within 2x of the \ | ||
| measured instant-proof signable length {measured}" | ||
| ); |
There was a problem hiding this comment.
🟡 Suggestion: Tighten the signable-length calibration assertion
This factor-of-two interval does not provide the calibration pin described by the test and handler comments. The measured fixture could grow from 390 to 780 bytes, doubling the production hash-block count from six to twelve, while the test would continue passing and the default quote would remain unchanged. Compare the fee-relevant 64-byte block counts so harmless byte-level variation within one pricing block remains allowed but any change to the charged block count forces an update.
| assert!( | |
| DEFAULT_SIGNABLE_BYTES_LEN_HINT >= measured / 2 | |
| && DEFAULT_SIGNABLE_BYTES_LEN_HINT <= measured.saturating_mul(2), | |
| "default hint {DEFAULT_SIGNABLE_BYTES_LEN_HINT} must stay within 2x of the \ | |
| measured instant-proof signable length {measured}" | |
| ); | |
| let sha256_block_size = u32::from( | |
| crate::execution::types::execution_operation::SHA256_BLOCK_SIZE, | |
| ); | |
| assert_eq!( | |
| DEFAULT_SIGNABLE_BYTES_LEN_HINT / sha256_block_size, | |
| measured / sha256_block_size, | |
| "default hint {DEFAULT_SIGNABLE_BYTES_LEN_HINT} and measured instant-proof \ | |
| signable length {measured} must have the same fee-relevant SHA-256 block count" | |
| ); |
source: ['codex']
fbc9836 to
571a00d
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The new fee-quote RPC is integrated through the versioned query stack, but all three previously verified findings remain at the exact head: non-canonical addresses are accepted, the hint bounds can underprice valid transitions, and the calibration test does not pin fee-relevant size drift. The latest snapshot-instability handling is retryable for the repository's Rust DAPI client, but it still converts an expected transient condition into gRPC UNKNOWN and logs it as a system failure, so changes remain required.
Source: reviewer backend openai/gpt-5.4 (Codex general, Rust-quality, and security lanes); final verifier backend anthropic/claude (exact model ID was not supplied to the verifier); orchestration-only openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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-drive-abci/src/query/address_funds/address_funding_fee_quote/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/query/address_funds/address_funding_fee_quote/v0/mod.rs:134-141: Preserve snapshot-instability semantics through gRPC mapping
Wrapping `CommittedStateChangedDuringOperation` in `QueryError::Drive` loses its transient classification at the service boundary. `query_error_into_status` maps unmatched errors to gRPC UNKNOWN, logs them as unexpected, and the query metrics path classifies UNKNOWN as a system error. The repository's Rust DAPI client does retry UNKNOWN, so retry itself is not broken there, but other clients cannot distinguish this expected busy-node condition and operators still receive the fault-level logging that the new branch explicitly intends to avoid. Introduce a dedicated transient query error mapped to `Status::unavailable` or another explicit retryable status, and cover the service-level status mapping.
- [BLOCKING] packages/rs-drive-abci/src/query/address_funds/address_funding_fee_quote/v0/mod.rs:62-68: Reject trailing bytes in serialized addresses
(existing thread: https://github.com/dashpay/platform/pull/4445#discussion_r3831666119)
`PlatformAddress::from_bytes` calls `bincode::decode_from_slice` but discards its consumed-byte count, so a valid 21-byte address followed by arbitrary trailing bytes is accepted. This contradicts the RPC's documented 21-byte format and malformed-address rejection contract. It also means the decoded canonical recipient is used for state lookup while the entire non-canonical request vector is hashed when deriving a placeholder outpoint, allowing multiple encodings of the same recipient to produce different quote paths. Require an exact canonical round trip before estimating.
- [BLOCKING] packages/rs-drive-abci/src/query/address_funds/address_funding_fee_quote/v0/mod.rs:31-33: Make signable-length bounds cover the valid fee range
(existing thread: https://github.com/dashpay/platform/pull/4445#discussion_r3831666155)
The current bounds contradict the API's guarantee that clamping prevents fee understatement. A nonzero hint below the measured 390-byte default is accepted as 128 bytes, reducing the production hash charge from six blocks to two. Hints above 8,192 bytes are also priced as 8,192 even though the protocol permits state transitions up to 20,480 bytes and asset-lock transactions with up to 100 inputs, allowing valid signable payloads above 8 KiB. Use the conservative measured default as the floor and cover the protocol's full valid size range; the ceiling should ultimately derive from `platform_version.system_limits.max_state_transition_size`.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs:10400-10405: Tighten the signable-length calibration assertion
(existing thread: https://github.com/dashpay/platform/pull/4445#discussion_r3831666169)
The factor-of-two interval does not enforce the calibration described by the test and handler comments. For example, the measured fixture could grow from 390 to 780 bytes, doubling the production hash charge from six blocks to twelve while this assertion continues to pass and the default remains unchanged. Compare the same 64-byte block counts used by production pricing so variation within one fee block remains harmless but fee-relevant drift requires an explicit update.
| Err(drive::error::Error::Drive( | ||
| error @ DriveError::CommittedStateChangedDuringOperation(_), | ||
| )) => { | ||
| // A transient condition on a busy node, not a server fault: | ||
| // surface it as a client-visible error so the caller retries. | ||
| return Ok(QueryValidationResult::new_with_error(QueryError::Drive( | ||
| drive::error::Error::Drive(error), | ||
| ))); |
There was a problem hiding this comment.
🟡 Suggestion: Preserve snapshot-instability semantics through gRPC mapping
Wrapping CommittedStateChangedDuringOperation in QueryError::Drive loses its transient classification at the service boundary. query_error_into_status maps unmatched errors to gRPC UNKNOWN, logs them as unexpected, and the query metrics path classifies UNKNOWN as a system error. The repository's Rust DAPI client does retry UNKNOWN, so retry itself is not broken there, but other clients cannot distinguish this expected busy-node condition and operators still receive the fault-level logging that the new branch explicitly intends to avoid. Introduce a dedicated transient query error mapped to Status::unavailable or another explicit retryable status, and cover the service-level status mapping.
source: ['codex']
571a00d to
7e37071
Compare
Add a narrow, read-only DAPI query that returns a state-aware fee quote for a 0-input/1-output AddressFundingFromAssetLock: the node prices the exact production operations with measured tree depths (the drive estimation engine from the previous commit), adds the same validation operations transform_into_action records (DoubleSha256 of the signable bytes with the identical integer division, one ECDSA_HASH160 verify — priced by the ORIGINAL add_many_to_fee_result, no replica), and applies the requested user_fee_increase. Request: recipient address, optional 36-byte asset lock outpoint (empty = a deterministic placeholder sha256d(tag || address || height); for a fresh lock both have the same expected search depth), user_fee_increase (<= u16::MAX), and a signable-bytes length hint clamped server-side to [128, 8192] with a measured 390-byte default so a client cannot understate the fee. Response: estimated_fee_credits, minimum_required_lock_credits (from the new shared calculate_address_funding_min_required_fee_for_counts in rs-dpp — the transition's calculate_min_required_fee now delegates to it, so the floor reported without a built transition can never drift), protocol_version, state_height, and standard metadata. The quote is a computed value, not state — the response deliberately carries no proof. A spent or partially used outpoint is refused with InvalidArgument (the quote models a fresh lock only), as are malformed addresses, wrong-size outpoints and oversized fee increases. rs-dapi proxies the new method via the existing drive_method! passthrough. Tests price the quote against real apply=true executions on the same committed state: genesis (quoted 12_503_540 vs actual 12_608_020, -0.8%), eight committed fundings (new recipient +2.0%, existing recipient replace +5.7% — and the replace quotes below the insert), user_fee_increase=14 (the SDK retry ceiling), placeholder == exact outpoint quote at genesis, spent-outpoint refusal, argument validation, read-only pin (grove root hash byte-identical across quotes) and determinism, and a fixture pin anchoring the default signable-length hint to the measured 390 bytes. Bands are [85%, 115%] regression headroom for these scenarios — not an upper-bound claim. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…isible quote error Map the engine's new CommittedStateChangedDuringOperation (committed state kept changing under the estimation's coherent-snapshot guard) to a query validation error instead of an internal server error: a transient condition on a busy node is the caller's cue to retry, not a fault worth alerting on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7e37071 to
14fdd09
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 14fdd09, the two blocking correctness issues and two suggestions from the prior review all remain. The RPC reuses the production estimator, but malformed address aliases and the 8 KiB hint ceiling can still produce ambiguous or understated quotes, preventing wallets from relying on the endpoint across its documented input range.
Source: reviewers openai/gpt-5.4 (Codex general, Rust-quality, and security-auditor); final verifier anthropic/claude (exact model ID was not supplied); orchestration-only openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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-drive-abci/src/query/address_funds/address_funding_fee_quote/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/query/address_funds/address_funding_fee_quote/v0/mod.rs:62-68: Reject trailing bytes in serialized addresses
(existing thread: https://github.com/dashpay/platform/pull/4445#discussion_r3831666119)
`PlatformAddress::from_bytes` delegates to `bincode::decode_from_slice` but discards the consumed-byte count, so a canonical 21-byte address followed by arbitrary bytes passes this validation. That violates the RPC's documented exact representation. It also uses two different interpretations of the same request: state lookup uses the decoded canonical address, while placeholder generation hashes the complete non-canonical request vector, allowing aliases of one recipient to select different estimation paths. Require the input to round-trip to its exact canonical encoding.
- [BLOCKING] packages/rs-drive-abci/src/query/address_funds/address_funding_fee_quote/v0/mod.rs:31-33: Make signable-length bounds cover the valid fee range
(existing thread: https://github.com/dashpay/platform/pull/4445#discussion_r3831666155)
The 8,192-byte ceiling is below the protocol's valid range. The active `max_state_transition_size` is 20,480 bytes, asset-lock transactions permit up to 100 inputs, and an instant proof embeds the transaction, so valid funding transitions can have signable payloads above 8 KiB. A caller supplying such a truthful length is forcibly priced at 8,192 bytes even though production computes the `DoubleSha256` charge from the actual signable length. The lower bound also does not support the documented claim that clamping prevents understatement: it accepts 128 bytes despite the measured instant-proof default being 390, while the request contains no payload with which to verify a claimed size. Derive the ceiling from the active platform version's transition-size limit and either use a defensible conservative floor or narrow the API's anti-understatement guarantee.
- [SUGGESTION] packages/rs-drive-abci/src/query/address_funds/address_funding_fee_quote/v0/mod.rs:134-141: Preserve snapshot-instability semantics through gRPC mapping
(existing thread: https://github.com/dashpay/platform/pull/4445#discussion_r3835959606)
This branch recognizes `CommittedStateChangedDuringOperation` as transient but wraps it in `QueryError::Drive`. `query_error_into_status` has no matching `Drive` arm, so it logs the expected condition as an unexpected error and returns gRPC `UNKNOWN`; the outer query path then classifies it as a system failure. The repository's Rust DAPI client retries `UNKNOWN`, but other clients cannot distinguish the busy-node condition and operators still receive fault-level logs. Map the exhausted snapshot retries to the existing `ResourceExhausted` query error, or add another explicitly retryable status, and cover that service-level mapping.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs:10400-10405: Tighten the signable-length calibration assertion
(existing thread: https://github.com/dashpay/platform/pull/4445#discussion_r3831666169)
The factor-of-two assertion does not enforce the fee-relevant calibration claimed by the test and handler comments. The measured fixture could grow from 390 to 780 bytes, doubling production's `DoubleSha256` block count while this test continued to pass with the unchanged default. Compare the same 64-byte block counts used by production so harmless drift within one pricing block remains allowed but any fee-changing drift requires an explicit calibration update.
2 participants
Issue being fixed or feature implemented
Second PR of the address-funding fee-quote stack (2/4, on top of #4444). Exposes the drive estimation engine as a narrow, read-only DAPI query so wallets can fetch a state-aware fee quote without any client-side GroveDB machinery.
What was done?
getAddressFundingFeeQuoteRPC (proto + drive-abci handler + query version bounds + rs-dapi passthrough). Request: recipient address, optional 36-byte asset lock outpoint (empty = deterministic placeholdersha256d(tag || address || height)— same expected search depth for a fresh lock),user_fee_increase(≤ u16::MAX), signable-bytes length hint clamped server-side to [128, 8192] with a measured 390-byte default (a client cannot understate the fee). Response:estimated_fee_credits,minimum_required_lock_credits,protocol_version,state_height, standard metadata — deliberately no proof (the quote is a computed value, not state).transform_into_actionrecords (DoubleSha256 with the identical integer division + one ECDSA_HASH160 verify), priced by the originaladd_many_to_fee_result— no replicated fee arithmetic — then applies the requesteduser_fee_increase.minimum_required_lock_creditscomes from the new sharedcalculate_address_funding_min_required_fee_for_countsin rs-dpp; the transition'scalculate_min_required_feenow delegates to it, so the floor reported without a built transition can never drift.InvalidArgument(the quote models a fresh lock only), as are malformed addresses, wrong-size outpoints, and oversized fee increases. Legacy JS DAPI needs no changes (it does not proxy the newer platform queries).How Has This Been Tested?
Seven new tests priced against real apply=true executions on the same committed state (all green; the full
address_fundingmodule = 119 tests passed):user_fee_increase = 14(the SDK retry ceiling) still brackets the real charged fee;Bands are [85%, 115%] regression headroom for these scenarios — not an upper-bound claim.
Breaking Changes
None — a new additive RPC; generated non-Rust clients are refreshed by the standard release tooling as with prior endpoint additions.
Checklist:
🤖 Generated with Claude Code