feat(platform): getAddressFundingFeeQuote query - #4445
Conversation
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>
|
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 |
|
✅ Review complete (commit faa108a) |
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']
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