feat(gas-keys): add send/exec fees for gas key actions#15067
feat(gas-keys): add send/exec fees for gas key actions#15067
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #15067 +/- ##
=======================================
Coverage 68.94% 68.95%
=======================================
Files 921 921
Lines 204023 204109 +86
Branches 204023 204109 +86
=======================================
+ Hits 140668 140741 +73
- Misses 57512 57519 +7
- Partials 5843 5849 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
darioush
force-pushed
the
darioush/gaskeys-txfees
branch
from
f2922e1 to
9dc0b0f
Compare
darioush
force-pushed
the
darioush/gaskeys-txfees
branch
2 times, most recently
from
6067953 to
4e957a7
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds send and execution fees for gas key actions (TransferToGasKey, WithdrawFromGasKey, and gas key variants of AddKey). The fee calculations account for storage read/write operations on gas keys and nonces, ensuring proper gas accounting for these new action types.
Changes:
- Added storage_read_gas and storage_write_gas helper functions to compute gas costs from ExtCosts with per-byte accounting
- Implemented send/exec fees for TransferToGasKey and WithdrawFromGasKey actions
- Extended permission_send_fees and permission_exec_fees to handle gas key permission variants with per-nonce storage write costs
- Added NONCE_VALUE_LEN constant and min_gas_key_borsh_len() helper for gas key size estimation
- Made access_key_key_len public and added gas_key_nonce_key_len helper for trie key length calculations
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| runtime/runtime/src/config.rs | Implements fee calculation logic for gas key actions, adding storage read/write gas helpers and integrating them into send/exec fee calculations |
| core/primitives/src/trie_key.rs | Exports access_key_key_len and adds gas_key_nonce_key_len helper for computing trie key lengths |
| core/primitives-core/src/account.rs | Adds NONCE_VALUE_LEN constant and min_gas_key_borsh_len() function for estimating gas key storage sizes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
runtime/runtime/src/config.rs
Outdated
| // Use minimum as an estimate for the value length. At the time of sending, | ||
| // we don't know the variable portion (FunctionCallPermission) of the | ||
| // specified gas key to transfer to. | ||
| // Note tx_cost is calculated at the time of sending. |
There was a problem hiding this comment.
The comment "Note tx_cost is calculated at the time of sending" is unclear in this context. The exec_fee function is already being called during transaction cost calculation, so this note doesn't provide additional clarity. Consider removing this comment or clarifying what specific aspect of tx_cost calculation timing is relevant here.
| // Note tx_cost is calculated at the time of sending. |
runtime/runtime/src/config.rs
Outdated
| base_fee.checked_add(all_bytes_fee).unwrap() | ||
| } | ||
| Transfer(_) => { | ||
| // TransferToGasKey has same send fees as a normal transfer. |
There was a problem hiding this comment.
as far as I understand we need to charge more for TransferToGasKey since it includes public_key field which makes it larger hence it should be more expensive
also transfer_send_fee accounts for implicit account creation, I guess we don't want that when transferring to gas keys?
There was a problem hiding this comment.
Implemented in b8e3651.
I switched to using fees.fee(ActionCosts::transfer) directly since we don't have implicit account creation with gas keys actions (checked by check_account_existence), and charged using new_data_receipt_byte for the public key.
darioush
force-pushed
the
darioush/gaskeys-txfees
branch
from
8deb444 to
b8e3651
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pugachAG
left a comment
pugachAG
left a comment
There was a problem hiding this comment.
please introduce dedicated costs instead of reusing the existing ones
runtime/runtime/src/config.rs
Outdated
| sender_is_receiver: bool, | ||
| public_key_len: usize, | ||
| ) -> Gas { | ||
| let transfer_fee = fees.fee(ActionCosts::transfer).send_fee(sender_is_receiver); |
There was a problem hiding this comment.
while reusing transafer cost makes sense, generally we do not do that and introduce dedicated costs with the same value
runtime/runtime/src/config.rs
Outdated
| // specified gas key to withdraw from. | ||
| let estimated_value_len = AccessKey::min_gas_key_borsh_len(); | ||
| gas_key_transfer_send_fee(fees, sender_is_receiver, action.public_key.len()) | ||
| .checked_add(storage_read_gas(ext, ak_key_len, estimated_value_len)) |
There was a problem hiding this comment.
same here, totally reasonable approach, but inconsistent with the rest of actions. for example please take a look at DeterministicStateInit. it also writes values to storage yet has dedicated deterministic_state_init_* costs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TransferToGasKey(_) => Gas::ZERO, | ||
| WithdrawFromGasKey(_) => Gas::ZERO, | ||
| WithdrawFromGasKey(action) => { | ||
| gas_key_transfer_send_fee(fees, sender_is_receiver, action.public_key.len()) |
There was a problem hiding this comment.
According to the PR description, WithdrawFromGasKey send fee should include "transfer send + new_data_receipt_byte x public_key.len() + storage_read + storage_write(gas key)" to account for reading and updating the gas key during send. However, the current implementation only charges transfer send + new_data_receipt_byte x public_key.len(), missing the storage_read and storage_write costs. This should be addressed with a separate function or additional charges for WithdrawFromGasKey send fees.
| gas_key_transfer_send_fee(fees, sender_is_receiver, action.public_key.len()) | |
| let base_fee = | |
| gas_key_transfer_send_fee(fees, sender_is_receiver, action.public_key.len()); | |
| let storage_read_fee = | |
| fees.fee(ActionCosts::storage_read_base).send_fee(sender_is_receiver); | |
| let storage_write_fee = | |
| fees.fee(ActionCosts::storage_write_base).send_fee(sender_is_receiver); | |
| base_fee | |
| .checked_add(storage_read_fee) | |
| .unwrap() | |
| .checked_add(storage_write_fee) | |
| .unwrap() |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let estimated_value_len = AccessKey::min_gas_key_borsh_len(); | ||
| gas_key_transfer_exec_fee(fees, ak_key_len, estimated_value_len) | ||
| } | ||
| WithdrawFromGasKey(action) => { | ||
| let ak_key_len = access_key_key_len(receiver_id, &action.public_key); | ||
| let estimated_value_len = AccessKey::min_gas_key_borsh_len(); |
There was a problem hiding this comment.
estimated_value_len uses AccessKey::min_gas_key_borsh_len(), which will undercharge gas_key_value_byte when the existing key is a GasKeyFunctionCall with non-empty method_names. Since the action updates and rewrites the whole AccessKey value, the fee estimate should use a safe upper bound (e.g., worst-case borsh length derived from config.wasm_config.limit_config) rather than the minimum.
| let estimated_value_len = AccessKey::min_gas_key_borsh_len(); | |
| gas_key_transfer_exec_fee(fees, ak_key_len, estimated_value_len) | |
| } | |
| WithdrawFromGasKey(action) => { | |
| let ak_key_len = access_key_key_len(receiver_id, &action.public_key); | |
| let estimated_value_len = AccessKey::min_gas_key_borsh_len(); | |
| let estimated_value_len = | |
| AccessKey::max_gas_key_borsh_len(&config.wasm_config.limit_config); | |
| gas_key_transfer_exec_fee(fees, ak_key_len, estimated_value_len) | |
| } | |
| WithdrawFromGasKey(action) => { | |
| let ak_key_len = access_key_key_len(receiver_id, &action.public_key); | |
| let estimated_value_len = | |
| AccessKey::max_gas_key_borsh_len(&config.wasm_config.limit_config); |
| let estimated_value_len = AccessKey::min_gas_key_borsh_len(); | ||
| gas_key_transfer_exec_fee(fees, ak_key_len, estimated_value_len) | ||
| } | ||
| WithdrawFromGasKey(action) => { | ||
| let ak_key_len = access_key_key_len(receiver_id, &action.public_key); | ||
| let estimated_value_len = AccessKey::min_gas_key_borsh_len(); |
There was a problem hiding this comment.
Same undercharging issue as TransferToGasKey: estimated_value_len uses the minimum gas-key access key borsh length, which can be significantly smaller than a GasKeyFunctionCall access key value. Consider using an upper-bound estimate so gas_key_value_byte can’t be bypassed by large function-call permissions.
| let estimated_value_len = AccessKey::min_gas_key_borsh_len(); | |
| gas_key_transfer_exec_fee(fees, ak_key_len, estimated_value_len) | |
| } | |
| WithdrawFromGasKey(action) => { | |
| let ak_key_len = access_key_key_len(receiver_id, &action.public_key); | |
| let estimated_value_len = AccessKey::min_gas_key_borsh_len(); | |
| let estimated_value_len = AccessKey::max_gas_key_borsh_len(); | |
| gas_key_transfer_exec_fee(fees, ak_key_len, estimated_value_len) | |
| } | |
| WithdrawFromGasKey(action) => { | |
| let ak_key_len = access_key_key_len(receiver_id, &action.public_key); | |
| let estimated_value_len = AccessKey::max_gas_key_borsh_len(); |
| sender_is_receiver: bool, | ||
| public_key_len: usize, | ||
| ) -> Gas { | ||
| let base_fee = fees.fee(ActionCosts::gas_key_transfer_base).send_fee(sender_is_receiver); |
There was a problem hiding this comment.
ActionCosts::gas_key_key_byte is used here to price receipt size (public key / GasKeyInfo borsh bytes) even though the same cost name is also used later for trie key byte costs in execution. This dual meaning is easy to misconfigure in the future; consider splitting into separate action cost parameters (receipt-byte vs trie-key-byte) or at least documenting that gas_key_key_byte intentionally matches new_data_receipt_byte on send.
| let base_fee = fees.fee(ActionCosts::gas_key_transfer_base).send_fee(sender_is_receiver); | |
| let base_fee = fees.fee(ActionCosts::gas_key_transfer_base).send_fee(sender_is_receiver); | |
| // NOTE: `gas_key_key_byte` is intentionally used here to price the *receipt size* | |
| // (public key / `GasKeyInfo` Borsh bytes). The same cost parameter is also used | |
| // elsewhere to price *trie key byte* costs during execution. This dual use is | |
| // deliberate: the configuration assumes that `gas_key_key_byte` on send matches | |
| // the per-byte receipt cost (e.g. `new_data_receipt_byte`), so changing this | |
| // value affects both receipt-size and trie-key-byte pricing. |
| WithdrawFromGasKey(action) => { | ||
| gas_key_transfer_send_fee(fees, sender_is_receiver, action.public_key.len()) | ||
| } |
There was a problem hiding this comment.
PR description/table says WithdrawFromGasKey send fee includes storage read+write on the gas key and exec fee is equivalent to transfer exec, but the implementation charges the same send fee as TransferToGasKey and also adds access-key trie key/value byte charges in exec. If the table is authoritative, adjust the fee split (or update the PR description) so spec and implementation match.
| /// Minimum borsh-serialized size of an AccessKey with a gas key permission. | ||
| /// This is the size for GasKeyFullAccess (the smallest gas key variant). | ||
| pub fn min_gas_key_borsh_len() -> usize { | ||
| borsh::object_length(&Self::gas_key_full_access(0)).unwrap() | ||
| } |
There was a problem hiding this comment.
min_gas_key_borsh_len() calls borsh::object_length(...) on every invocation. This function is used in fee computation hot paths, so consider making this a const/hard-coded size (if stable) or caching it in a static LazyLock to avoid repeated serialization work.
| pub fn borsh_len() -> usize { | ||
| borsh::object_length(&Self { balance: Balance::from_yoctonear(0), num_nonces: 0 }).unwrap() |
There was a problem hiding this comment.
GasKeyInfo::borsh_len() recomputes borsh::object_length(...) each call. Since this is a fixed-size struct, consider making this a const or cached value to avoid repeated serialization during fee calculation.
| pub fn borsh_len() -> usize { | |
| borsh::object_length(&Self { balance: Balance::from_yoctonear(0), num_nonces: 0 }).unwrap() | |
| /// Borsh-serialized size of `GasKeyInfo`. | |
| /// This struct consists only of fixed-size numeric fields, so its Borsh | |
| /// representation has a fixed size known at compile time. | |
| pub const BORSH_LEN: usize = | |
| std::mem::size_of::<Balance>() + std::mem::size_of::<NonceIndex>(); | |
| pub fn borsh_len() -> usize { | |
| Self::BORSH_LEN |
|
Based on the feedback, the new derivations are (will update PR description at end): Send Fees AddKey (gas key variant) - permission_send_fees: Exec Fees AddKey (gas key variant) - permission_exec_fees: Values (derived from ExtCosts) |
2 participants
In NEP-611, it is specified:
This PR provides the concrete implementation for this specification.
TransferToGasKeynew_data_receipt_bytexpublic_key.len()Rationale: Charges for extra receipt size from the
public_keyfield.storage_read+storage_write(gas key)Rationale: transfer exec covers reading account (also pays for updating account, but it seems fine to overcharge this portion of a small fee). Adding the cost of updating the gas key to transfer to.
WithdrawFromGasKeynew_data_receipt_bytexpublic_key.len()+storage_read+storage_write(gas key)Rationale: Charges for extra receipt size from the
public_keyfield. Adding the cost of reading and updating the gas key to withdraw from.Rationale: depositing to account, should be same as transfer
GasKeyFullAccessadd_full_access_keysendadd_full_access_keyexec + N xstorage_write(nonce)GasKeyFunctionCalladd_function_call_keysendadd_function_call_keyexec + N xstorage_write(nonce)DeleteKey & DeleteAccount can only charge variable compute for num nonces, which I will address in a separate PR.
N =
num_noncesstorage_read_gas/storage_write_gashelpers inconfig.rscomputing gas fromExtCostswith per-byte accountingAccessKey::NONCE_VALUE_LEN,min_gas_key_borsh_len(), and trie key length helpers as shared primitivesmin_gas_key_borsh_len()as estimated value length where the actual gas key variant is unknown at fee computation time