Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Commit

Permalink
Fix sp-api handling of multiple arguments (#6484)
Browse files Browse the repository at this point in the history
With the switch to `decode_all_with_depth_limit` we silently broken
support for functions with multiple arguments. The old generated code
tried to decode each parameter separately, which does not play well with
`decode_all`.

This pr adds a test to ensure that this does not happen again and fixes
the bug by decoding everything at once by wrapping it into tuples.
  • Loading branch information
bkchr authored Jun 23, 2020
1 parent 6221146 commit c771821
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 11 deletions.
19 changes: 8 additions & 11 deletions primitives/api/proc-macro/src/impl_runtime_apis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use syn::{
fold::{self, Fold}, parse_quote,
};

use std::{collections::HashSet, iter};
use std::collections::HashSet;

/// Unique identifier used to make the hidden includes unique for this macro.
const HIDDEN_INCLUDES_ID: &str = "IMPL_RUNTIME_APIS";
Expand Down Expand Up @@ -71,26 +71,23 @@ fn generate_impl_call(
let params = extract_parameter_names_types_and_borrows(signature, AllowSelfRefInParameters::No)?;

let c = generate_crate_access(HIDDEN_INCLUDES_ID);
let c_iter = iter::repeat(&c);
let fn_name = &signature.ident;
let fn_name_str = iter::repeat(fn_name.to_string());
let input = iter::repeat(input);
let fn_name_str = fn_name.to_string();
let pnames = params.iter().map(|v| &v.0);
let pnames2 = params.iter().map(|v| &v.0);
let ptypes = params.iter().map(|v| &v.1);
let pborrow = params.iter().map(|v| &v.2);

Ok(
quote!(
#(
let #pnames : #ptypes = match #c_iter::DecodeLimit::decode_all_with_depth_limit(
#c_iter::MAX_EXTRINSIC_DEPTH,
&mut #input
let (#( #pnames ),*) : ( #( #ptypes ),* ) =
match #c::DecodeLimit::decode_all_with_depth_limit(
#c::MAX_EXTRINSIC_DEPTH,
&mut #input,
) {
Ok(input) => input,
Ok(res) => res,
Err(e) => panic!("Bad input data provided to {}: {}", #fn_name_str, e.what()),
};
)*

#[allow(deprecated)]
<#runtime as #impl_trait>::#fn_name(#( #pborrow #pnames2 ),*)
Expand Down Expand Up @@ -138,7 +135,7 @@ fn generate_impl_calls(

/// Generate the dispatch function that is used in native to call into the runtime.
fn generate_dispatch_function(impls: &[ItemImpl]) -> Result<TokenStream> {
let data = Ident::new("data", Span::call_site());
let data = Ident::new("__sp_api__input_data", Span::call_site());
let c = generate_crate_access(HIDDEN_INCLUDES_ID);
let impl_calls = generate_impl_calls(impls, &data)?
.into_iter()
Expand Down
11 changes: 11 additions & 0 deletions primitives/api/test/tests/runtime_calls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,3 +207,14 @@ fn record_proof_works() {
&runtime_code,
).expect("Executes block while using the proof backend");
}

#[test]
fn call_runtime_api_with_multiple_arguments() {
let client = TestClientBuilder::new().set_execution_strategy(ExecutionStrategy::Both).build();

let data = vec![1, 2, 4, 5, 6, 7, 8, 8, 10, 12];
let block_id = BlockId::Number(client.chain_info().best_number);
client.runtime_api()
.test_multiple_arguments(&block_id, data.clone(), data.clone(), data.len() as u32)
.unwrap();
}
16 changes: 16 additions & 0 deletions test-utils/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,9 @@ cfg_if! {
fn test_ecdsa_crypto() -> (ecdsa::AppSignature, ecdsa::AppPublic);
/// Run various tests against storage.
fn test_storage();
/// Test that ensures that we can call a function that takes multiple
/// arguments.
fn test_multiple_arguments(data: Vec<u8>, other: Vec<u8>, num: u32);
}
}
} else {
Expand Down Expand Up @@ -359,6 +362,9 @@ cfg_if! {
fn test_ecdsa_crypto() -> (ecdsa::AppSignature, ecdsa::AppPublic);
/// Run various tests against storage.
fn test_storage();
/// Test that ensures that we can call a function that takes multiple
/// arguments.
fn test_multiple_arguments(data: Vec<u8>, other: Vec<u8>, num: u32);
}
}
}
Expand Down Expand Up @@ -641,6 +647,11 @@ cfg_if! {
test_read_storage();
test_read_child_storage();
}

fn test_multiple_arguments(data: Vec<u8>, other: Vec<u8>, num: u32) {
assert_eq!(&data[..], &other[..]);
assert_eq!(data.len(), num as usize);
}
}

impl sp_consensus_aura::AuraApi<Block, AuraId> for Runtime {
Expand Down Expand Up @@ -862,6 +873,11 @@ cfg_if! {
test_read_storage();
test_read_child_storage();
}

fn test_multiple_arguments(data: Vec<u8>, other: Vec<u8>, num: u32) {
assert_eq!(&data[..], &other[..]);
assert_eq!(data.len(), num as usize);
}
}

impl sp_consensus_aura::AuraApi<Block, AuraId> for Runtime {
Expand Down

0 comments on commit c771821

Please sign in to comment.