Dispatch Lockdrop account - Precompile #1142
Conversation
PierreOssun
changed the title
There was a problem hiding this comment.
After going through the code I realised few things,
- Instead of taking the whole signature (signed message) as input and extract the pub key inside the precompile, we should just take a pub key as input and just add validation of
keccak_256(pub_key)[..20] = evm_caller. - That way we don't have to build EIP signature payload, verify signature and extract the secp256k1 key inside the precompile code which we can't properly benchmark.
- The UI can handle this very easily in one line using existing eth libraries like ethers.js -
recoverPublicKey(do note that UI already has the logic to produce payload and signature before hand, so it's like preventing double work also)
There was a problem hiding this comment.
Looks good! except for some minor comments
I couldn't find .sol interface file for the precompile and overlooked in my previous review. The original dispatch precompile take only one input (encoded call) and without a method selector (fallback routine) so it was fine for it, but for our precompile IMO it is needed as we take more than one input and has explicit function selector.(dispatch_lockdrop_call(bytes,bytes))
| return Err(PrecompileFailure::Error { | ||
| exit_status: ExitError::Other(message.into()), | ||
| }); |
There was a problem hiding this comment.
| return Err(PrecompileFailure::Error { | |
| exit_status: ExitError::Other(message.into()), | |
| }); | |
| return Err(revert("caller does not match the public key"); |
| Err(PrecompileFailure::Error { | ||
| exit_status: ExitError::Other( | ||
| format!("dispatch execution failed: {}", <&'static str>::from(e)).into(), | ||
| ), | ||
| }) |
There was a problem hiding this comment.
You can use revert() here as well to keep it consistent
| let caller: H160 = handle.context().caller.into(); | ||
| let input: Vec<u8> = call.into(); | ||
|
|
||
| // 1. Decode the call |
There was a problem hiding this comment.
I'm not sure how expensive all these operations are (ignoring the call dispatch), but I'd suggest to add some static cost at the beginning, that's always charged. Sort of a deterrent to prevent spam attack and continued use of this precompile.
Correct me if I'm wrong, but we just want users to get their funds out and continue using "normal" ways of interacting with the chain.
There was a problem hiding this comment.
How gas is handled:
- Check if call can be made (not exceed gas limit)
- Charge external cost: call weight + hash weight
- If call is dispatched:
- Charge actual cost of the runtime call
- Refund the difference between the actual cost and the estimated cost (not refunding the hash cost)
So user only pays for the actual weight of the dispatched calls + overhead (2 hashing instructions)
There was a problem hiding this comment.
Ok, I think I understand where we're misunderstanding each other.
I see two problems:
- Double charging
- Incorrect charging
Double charging
The first one, double charging, happens because as you outlined your steps:
2. Charge external cost
3.1. Charge actual cost
3.2. Refund the initial charged weight
After the 2nd step, you have double charged.
It's true that you refund it later, but at that point the total charged amount might have exceeded what would otherwise be good enough good limit if it weren't for temporary double charging.
Incorrect charging
Later on, when you call:
let actual_weight = post_info.actual_weight.unwrap_or(info.weight);
let cost = Runtime::GasWeightMapping::weight_to_gas(actual_weight);
handle.record_cost(cost)?;you will actually ignore the PoV part of the weight.
fn weight_to_gas(weight: Weight) -> u64 {
weight.div(T::WeightPerGas::get().ref_time()).ref_time()
}This is the approach used by try_dispatch:
pub fn try_dispatch<Call>(
handle: &mut impl PrecompileHandle,
origin: <Runtime::RuntimeCall as Dispatchable>::RuntimeOrigin,
call: Call,
) -> Result<PostDispatchInfo, TryDispatchError>
where
Runtime::RuntimeCall: From<Call>,
{
let call = Runtime::RuntimeCall::from(call);
let dispatch_info = call.get_dispatch_info();
Self::record_weight_v2_cost(handle, dispatch_info.weight).map_err(TryDispatchError::Evm)?;
// Dispatch call.
// It may be possible to not record gas cost if the call returns Pays::No.
// However while Substrate handle checking weight while not making the sender pay for it,
// the EVM doesn't. It seems this safer to always record the costs to avoid unmetered
// computations.
let post_dispatch_info = using_precompile_handle(handle, || call.dispatch(origin))
.map_err(|e| TryDispatchError::Substrate(e.error))?;
Self::refund_weight_v2_cost(
handle,
dispatch_info.weight,
post_dispatch_info.actual_weight,
)
.map_err(TryDispatchError::Evm)?;
Ok(post_dispatch_info)
}
}Suggestion
- At the very beginning of the call you charge the hardcoded/fixed value to cover for all the decoding, hashing, etc. This is purely computation (ref_time, gas), no PoV is involved.
- When you get to the part when it's time to dispatch the call, use the approach from
try_dispatch, which records both weight components.
There was a problem hiding this comment.
- Double Charging: I don't see any purpose for that
record_costcall, it can be removed completely as it doesn't really make sense to double charge ref_time. - Incorrect charging: We should probably charge a hefty fee from users using this precompile, as Dino said we just want users to get their funds out and continue using "normal" ways of interacting with the chain,. Charging a big fee will surely discourage people using it for other purposes.
There was a problem hiding this comment.
I've checked the changes, and they look good to me!
Left just one comment about charged ref time.
Suggestion - add an unit test or two to make sure expected weight is charged.
Given how much discussions we had about it, it would be good to have a UT ensure it's behaving as we now expect 🙂
|
|
||
| // Derive the account id from the public key | ||
| let origin = Self::get_account_id_from_pubkey(pubkey.as_bytes()) | ||
| .ok_or(revert("could not derive AccountId from pubkey"))?; |
There was a problem hiding this comment.
Is it possible to cover this case in unit tests?
There was a problem hiding this comment.
Added a tests for it. Using the same pubkey & AccountId as from old pallet eth-sig
Astar/pallets/custom-signatures/src/tests.rs
Line 267 in 9395ce0
Minimum allowed line rate is |
Pull Request Summary
Added a precompile
dispatch-lockdropthat exposedispatch_lockdrop_callfunction in order to dispatch calls on behalf of SS58 AccountId of the lockdrop account (in the same way it was derived ineth-sig).The precompile is only accessible from EOA and proceed to some checks before disptaching the call.
It used it's own CallFilter (allow the same call as Dispatch)