feat(universal-accounts): add UniversalStateInit action and handler - #16103
feat(universal-accounts): add UniversalStateInit action and handler#16103Wiezzel wants to merge 2 commits into
Conversation
| InvalidUniversalStateInitReceiver { | ||
| receiver_id: AccountId, | ||
| derived_id: AccountId, | ||
| } = 22, | ||
| /// A `UniversalStateInit` state init defines neither contract code nor an | ||
| /// access key, so the resulting account could never be used. | ||
| UnusableUniversalStateInit = 23, | ||
| /// A storage key in a `UniversalStateInit` state init exceeds the limit. | ||
| UniversalStateInitKeyLengthExceeded { | ||
| length: u64, | ||
| limit: u64, | ||
| } = 24, | ||
| /// A storage value in a `UniversalStateInit` state init exceeds the limit. | ||
| UniversalStateInitValueLengthExceeded { | ||
| length: u64, | ||
| limit: u64, | ||
| } = 25, |
There was a problem hiding this comment.
I considered re-using the existing DeterministicStateInit error variants here, but that would be a backwards-incompatible RPC change: ActionsValidationError serializes the variant name as the JSON tag, and the deterministic variants are live (stable since v82), so sharing or renaming them would change the error strings clients match on. Borsh is unaffected (it's keyed on the discriminant). Hence the parallel Universal* variants; a neutral-name consolidation can be done deliberately in a follow-up PR.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #16103 +/- ##
==========================================
+ Coverage 73.69% 73.71% +0.02%
==========================================
Files 864 865 +1
Lines 192209 192529 +320
Branches 192209 192529 +320
==========================================
+ Hits 141642 141917 +275
- Misses 46085 46119 +34
- Partials 4482 4493 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
61c23ff to
c884b35
Compare
|
cc @mitinarseny |
Pull request overviewAdds a new Changes:
Reviewed changesPer-file summary
FindingsNon-blocking (suggestions / follow-ups):
Existing thread on ✅ Approved |
|
Do we need a changelog entry or will it be done with the stabilization PR? |
Trisfald
left a comment
Trisfald
left a comment
There was a problem hiding this comment.
Double checking: in this PR 0u has no AccountType variant, and as such CreateAccount isn't guarded and Transfer can't pre-fund.
Do you plan to enable implicit UA in a follow up PR?
| Ok(()) | ||
| } | ||
|
|
||
| fn validate_universal_state_init( |
There was a problem hiding this comment.
I wonder if we should have a limit on the number of data entries or access keys? Maybe many short ones can be an attack angle
There was a problem hiding this comment.
There MUST be no limit on the number of entries, at least, as long as protocol itself doesn't have one. If no, then it MUST succeed as long as the StateInit's deposit + account's balance is enough to cover for storage staking.
| UniversalStateInit { | ||
| code: Option<GlobalContractIdentifierView>, | ||
| #[serde_as(as = "BTreeMap<Base64, Base64>")] | ||
| #[cfg_attr(feature = "schemars", schemars(with = "BTreeMap<String, String>"))] | ||
| data: BTreeMap<Vec<u8>, Vec<u8>>, | ||
| access_keys: Vec<PublicKeyHandle>, | ||
| deposit: Balance, | ||
| } = 17, |
There was a problem hiding this comment.
This is not versioned and creates inconsistency with the Action::UniversalStateInit layout. Can we reuse UniversalStateInit here?
| UniversalStateInit { | |
| code: Option<GlobalContractIdentifierView>, | |
| #[serde_as(as = "BTreeMap<Base64, Base64>")] | |
| #[cfg_attr(feature = "schemars", schemars(with = "BTreeMap<String, String>"))] | |
| data: BTreeMap<Vec<u8>, Vec<u8>>, | |
| access_keys: Vec<PublicKeyHandle>, | |
| deposit: Balance, | |
| } = 17, | |
| UniversalStateInit { | |
| state_init: UniversalStateInit, | |
| deposit: Balance, | |
| } = 17, |
There was a problem hiding this comment.
I somewhat blindly followed the precedent of DeterministicStateInit which is also a versioned struct, but also does this kind of flattening. Let me check if there was any good reason deterministic account were done that way.
There was a problem hiding this comment.
@jakmeier Could explain why was ActionView::DeterministicStateInit implemented as it was, instead of re-suing the versioned struct?
| let all_entries_fee = entry_fee.checked_mul(num_entries).unwrap(); | ||
| let all_bytes_fee = byte_fee.checked_mul(num_bytes).unwrap(); | ||
| let all_keys_fee = key_fee.checked_mul(num_keys).unwrap(); | ||
| base_fee | ||
| .checked_add(all_bytes_fee) | ||
| .unwrap() | ||
| .checked_add(all_entries_fee) | ||
| .unwrap() | ||
| .checked_add(all_keys_fee) | ||
| .unwrap() |
There was a problem hiding this comment.
Are you sure it's ok to panic on overflows here?
| // A `0u` account can only come into existence through this action, so | ||
| // an account that already exists here is the one this action initialized. | ||
| // Initialize on first sight; on repeat, skip straight to the deposit | ||
| // handling without touching the installed state. | ||
| let needs_init = maybe_account.is_none(); | ||
| let account = match maybe_account { | ||
| Some(account) => account, | ||
| // Create without changing actor_id, so a same-receipt follow-up can't hijack the account. | ||
| None => maybe_account.insert(Account::new( | ||
| Balance::ZERO, | ||
| Balance::ZERO, | ||
| AccountContract::None, | ||
| storage_usage_config.num_bytes_account, | ||
| )), | ||
| }; | ||
|
|
||
| if needs_init { | ||
| install_universal_account( | ||
| state_update, | ||
| account, | ||
| account_id, | ||
| &action.state_init, | ||
| result, | ||
| fees, | ||
| apply_state.block_height, | ||
| )?; | ||
| if result.result.is_err() { | ||
| return Ok(()); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is not true: the account can be "created" by receiving an incoming transfer first. StateInit should only be applied (i.e. "installed") if and only if the account didn't exist yet or if its initialized flag is unset.
| // A `0u` account can only come into existence through this action, so | |
| // an account that already exists here is the one this action initialized. | |
| // Initialize on first sight; on repeat, skip straight to the deposit | |
| // handling without touching the installed state. | |
| let needs_init = maybe_account.is_none(); | |
| let account = match maybe_account { | |
| Some(account) => account, | |
| // Create without changing actor_id, so a same-receipt follow-up can't hijack the account. | |
| None => maybe_account.insert(Account::new( | |
| Balance::ZERO, | |
| Balance::ZERO, | |
| AccountContract::None, | |
| storage_usage_config.num_bytes_account, | |
| )), | |
| }; | |
| if needs_init { | |
| install_universal_account( | |
| state_update, | |
| account, | |
| account_id, | |
| &action.state_init, | |
| result, | |
| fees, | |
| apply_state.block_height, | |
| )?; | |
| if result.result.is_err() { | |
| return Ok(()); | |
| } | |
| } | |
| let account = match maybe_account { | |
| Some(account) => account, | |
| // Create without changing actor_id, so a same-receipt follow-up can't hijack the account. | |
| None => maybe_account.insert(Account::new( | |
| Balance::ZERO, | |
| Balance::ZERO, | |
| AccountContract::None, | |
| storage_usage_config.num_bytes_account, | |
| )), | |
| }; | |
| if !account.is_initialized() { | |
| install_universal_account( | |
| state_update, | |
| account, | |
| account_id, | |
| &action.state_init, | |
| result, | |
| fees, | |
| apply_state.block_height, | |
| )?; | |
| if result.result.is_err() { | |
| return Ok(()); | |
| } | |
| } |
| // Mirror `access_key_storage_usage`: on-trie handle length + the access | ||
| // key's borsh length + the per-record overhead. | ||
| let key_bytes = (handle.trie_id_len() as u64) |
There was a problem hiding this comment.
Does it mean that smart-contracts will have to hardcode this trie_id_len value as well if they want to estimate deposit required for given StateInit?
Add the `UniversalStateInit` action (discriminant 15), which creates a `0u` universal account on-chain from its state init: optional contract code, storage entries, and full-access keys. It mirrors `DeterministicStateInit` but also installs access keys and supports key-only (code-less) accounts. - Gate behind a new nightly `ProtocolFeature::UniversalAccounts`. - Runtime handler in `universal_account_id.rs`, sharing the deposit settlement and data-length helpers with the deterministic handler. - Action validation: feature gate, validity rule, derived-id check, and per-entry key/value limits, plus new `ActionsValidationError` variants. - Dedicated `universal_state_init_*` fee params (values mirror the deterministic action; each access key is charged as an `add_full_access_key`). No estimator yet; the config-store fallback is used and calibration is deferred to a follow-up PR. - `ActionView` variant and conversions; protocol schema and OpenAPI regenerated. - Validation unit test plus test-loop tests (key-only, contract, and repeated init).
c884b35 to
d42d2a6
Compare
Greptile SummaryAdds the protocol-gated
Confidence Score: 4/5The PR appears safe to merge after correcting the non-blocking RPC schema mismatch for empty universal state initialization. Runtime execution, charging, storage settlement, feature gating, and pipeline integration are internally consistent, but the generated API specifications expose a validation error that the implementation intentionally never returns. Files Needing Attention: chain/jsonrpc/openapi/openapi.json, chain/jsonrpc/openapi/openrpc.json
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }, | ||
| { | ||
| "description": "A `UniversalStateInit` state init defines neither contract code nor an\naccess key, so the resulting account could never be used.", | ||
| "enum": [ | ||
| "UnusableUniversalStateInit" | ||
| ], | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
The generated OpenAPI and OpenRPC schemas advertise UnusableUniversalStateInit, but the runtime has no corresponding error variant and explicitly accepts an initialization containing neither code nor access keys. Generated clients therefore expose and may handle an error that the API can never return.
3 participants
Adds the
UniversalStateInitaction, which creates a0uuniversal account on-chain from its state init: optional contract code, initial storage, and full-access keys. It mirrors the existingDeterministicStateInitaction but additionally installs access keys and supports key-only (code-less) accounts. The action is gated behind a new nightlyProtocolFeature::UniversalAccounts.universal_state_init_*fee parameters; each installed key is charged as anadd_full_access_key. Values currently mirror the deterministic action and are not yet estimator-calibrated (config-store fallback), left to a follow-up PR.ActionViewvariant plus conversions; protocol schema and OpenAPI/OpenRPC regenerated.Note: because universal accounts closely parallel deterministic accounts, several pieces here are very similar to or outright copied from the deterministic state-init code (validation, the runtime handler flow, the fee shape). Some shared logic is already deduplicated in this PR; the remaining overlap (e.g. folding the install/deploy handlers and consolidating the validators) is intentionally left for a follow-up clean-up PR, to keep this change's blast radius off the stable deterministic path.