feat(node): enforce TIP-1011 restrictions#3386
feat(node): enforce TIP-1011 restrictions#3386legion2002 merged 4 commits intotanishk/tip-1011-precompiles-exactfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b1f5476ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
476b865 to
9f1ed91
Compare
9f1ed91 to
5434753
Compare
b609885 to
e5c4a35
Compare
| let authorize_call = authorizeKeyCall { | ||
| keyId: access_key_addr, | ||
| signatureType: signature_type, | ||
| expiry, | ||
| enforceLimits: enforce_limits, | ||
| limits: precompile_limits, | ||
| config: KeyRestrictions { | ||
| expiry, | ||
| enforceLimits: enforce_limits, | ||
| limits: precompile_limits, | ||
| enforceAllowedCalls: enforce_allowed_calls, | ||
| allowedCalls: precompile_allowed_calls, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
it probably doesn't matter much, but for perf reasons, i think fn authorize_key (and any other precompile fn used by the handler) shouldn't generally require the SolCall struct, as it is deconstructed in the precompile impl
imo it would be better to flatten the fn sig of fn authorize_key to avoid useless conversions
it may be optimized by the compiler, but feels cleaner (cc: @klkvr in case u are opinionated or think this proposal is not useful)
5434753 to
42d8683
Compare
f5c06cf to
e92a22e
Compare
| recipients.contains(&Address::from_slice(&recipient_word[12..])) | ||
| } | ||
|
|
||
| fn validate_inline_t3_call_scopes( |
There was a problem hiding this comment.
nit: can we add doc comments to this function?
| Ok(()) | ||
| } | ||
|
|
||
| fn validate_t3_key_authorization_shape( |
There was a problem hiding this comment.
nit: can we add doc comments on this function?
| "duplicate token limits are not allowed", | ||
| )); | ||
| } | ||
|
|
||
| if limit.limit > U256::from(u128::MAX) { | ||
| return Err(TempoPoolTransactionError::Keychain( | ||
| "spending limit exceeds u128::MAX", | ||
| )); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let Some(scopes) = auth.allowed_calls.as_ref() else { | ||
| return Ok(()); | ||
| }; | ||
|
|
||
| if scopes.len() > MAX_CALL_SCOPES as usize { | ||
| return Err(TempoPoolTransactionError::Keychain( | ||
| "too many call scopes in key authorization", | ||
| )); | ||
| } | ||
|
|
||
| let mut seen_targets = HashSet::with_capacity(scopes.len()); | ||
| for scope in scopes { | ||
| if !seen_targets.insert(scope.target) { | ||
| return Err(TempoPoolTransactionError::Keychain( | ||
| "duplicate call scope targets are not allowed", | ||
| )); | ||
| } | ||
|
|
||
| let Some(selector_rules) = scope.selector_rules.as_ref() else { | ||
| continue; |
There was a problem hiding this comment.
Can we also add some lighweight comments here detailing what each group of checks are doing?
| fn call_scope_storage_slots(auth: &tempo_primitives::transaction::KeyAuthorization) -> u64 { | ||
| match auth.allowed_calls.as_ref() { | ||
| None => 0, | ||
| Some(scopes) if scopes.is_empty() => 1, | ||
| Some(scopes) => { |
There was a problem hiding this comment.
Looks like the comments for calculate_key_authorization_gas were not moved with the function. Can we move the comments to the correct function and add new doc comments for call_scope_storage_slots.
crates/revm/src/handler.rs
Outdated
| let precompile_allowed_calls = key_auth | ||
| .allowed_calls | ||
| .as_ref() | ||
| .map(|scopes| { | ||
| scopes | ||
| .iter() | ||
| .filter_map(|scope| match scope.selector_rules.as_ref() { | ||
| Some(rules) if rules.is_empty() => None, | ||
| _ => Some(tempo_precompiles::account_keychain::CallScope { | ||
| target: scope.target, | ||
| selectorRules: scope | ||
| .selector_rules | ||
| .as_ref() | ||
| .map(|rules| { | ||
| rules | ||
| .iter() | ||
| .map(|rule| { | ||
| tempo_precompiles::account_keychain::SelectorRule { | ||
| selector: rule.selector.into(), | ||
| recipients: rule | ||
| .recipients | ||
| .clone() | ||
| .unwrap_or_default(), | ||
| } | ||
| }) | ||
| .collect() | ||
| }) | ||
| .unwrap_or_default(), | ||
| }), | ||
| }) |
There was a problem hiding this comment.
nit: consider making this a bit more readable.
e92a22e to
3a6c0bb
Compare
a29e4af to
e51b9cf
Compare
3a6c0bb to
f2e4a1f
Compare
73d8adb
into
tanishk/tip-1011-precompiles-exact
## Stack Context This is the middle PR in the TIP-1011 stack. - Parent: [#3384](#3384) `feat(primitives): add TIP-1011 wire format and ABI` - Child: [#3386](#3386) `feat(node): enforce TIP-1011 restrictions` ## What This PR Does This PR implements TIP-1011 inside the `AccountKeychain` precompile: - periodic spending-limit state and reset behavior - allowed-call target / selector / recipient scoping - precompile mutators and readers for the new restriction state - hardfork-gated dispatch and direct readers that depend on the new layout This is the PR where the spec becomes concrete precompile and storage behavior. It intentionally stops short of handler and txpool runtime enforcement, which lands in PR3. ## How To Review Review this like a smart-contract PR. 1. Start in `crates/precompiles/src/account_keychain/mod.rs` and verify the storage model and invariants: uniqueness, rollover behavior, normalization, selector and recipient validation, and any assumptions about reads and writes. 2. Look for efficiency opportunities as you go: slot count, tight packing, unnecessary writes, and whether the chosen layout is the right tradeoff for expected access patterns. 3. Check access gating carefully for the important mutators, especially `authorizeKey`, `setAllowedCalls`, `removeAllowedCalls`, and `updateSpendingLimit`. 4. Review `crates/precompiles/src/account_keychain/dispatch.rs` for ABI routing and fork gating. 5. As a final pass, think ahead to how the handler will interact with these precompile functions and where they will be used in [#3386](#3386). 6. Related context: [#3370](#3370) is a draft PR that adds an enumerable map utility and makes the `targets` / `target_scopes` path more efficient. It is worth keeping in mind when evaluating this layout. ## Note If you want the full end-to-end diff in one place, see [#3323](#3323). It is useful for seeing how the ideas interact across the stack, and Cyclops also runs there.
Stack Context
This is the top PR in the TIP-1011 stack.
feat(precompiles): implement TIP-1011 restrictionsWhat This PR Does
This PR enforces TIP-1011 at runtime:
key_authorizationThis is the integration layer where the protocol shape from PR1 and the precompile semantics from PR2 are actually enforced by the node.
How To Review
crates/revm/src/handler.rs. This is the core of the PR: hardfork gating, intrinsic gas, same-transaction authorization behavior, and per-call enforcement.crates/transaction-pool/src/validator.rsand confirm txpool admission matches the runtime rules closely enough to reject bad transactions early without diverging from execution.Note
If you want the full end-to-end diff in one place, see #3323. It is useful for seeing how the ideas interact across the stack, and Cyclops also runs there.