From 358a184da3b28612273ebf42767df04875213b82 Mon Sep 17 00:00:00 2001 From: Sean Young Date: Wed, 20 Dec 2023 15:59:29 +0000 Subject: [PATCH] Fix abi.encodeCall() argument parsing (#1612) The arguments to the function should be passed as a tuple, if there are more than one. A single argument does not require a tuple. Signed-off-by: Sean Young --- .github/workflows/test.yml | 20 +- docs/examples/abi_encode_call.sol | 2 +- docs/language/builtins.rst | 15 +- src/codegen/mod.rs | 2 +- src/sema/builtin.rs | 228 ++++++++---------- src/sema/expression/mod.rs | 22 +- src/sema/namespace.rs | 2 + src/sema/statements.rs | 2 +- tests/contract_testcases/evm/bytes_cast.sol | 16 ++ .../polkadot/builtins/abi_decode_02.sol | 2 +- .../polkadot/builtins/abi_decode_03.sol | 2 +- .../solana/call/abi_encode_call.sol | 31 ++- tests/evm.rs | 2 +- tests/solana_tests/call.rs | 31 +++ 14 files changed, 212 insertions(+), 165 deletions(-) create mode 100644 tests/contract_testcases/evm/bytes_cast.sol diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b1ac6104c..3f30af558 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -356,9 +356,6 @@ jobs: - name: Start substrate contracts node run: echo id=$(docker run -d -p 9944:9944 ghcr.io/hyperledger/solang-substrate-ci:054bef6 substrate-contracts-node --dev --rpc-external) >> $GITHUB_OUTPUT id: substrate - - uses: actions/setup-node@v3 - with: - node-version: '16' - uses: actions/download-artifact@v3 with: name: solang-linux-x86-64 @@ -366,6 +363,9 @@ jobs: - run: | chmod 755 ./bin/solang echo "$(pwd)/bin" >> $GITHUB_PATH + - uses: actions/setup-node@v3 + with: + node-version: '16' - run: npm install working-directory: ./integration/polkadot - name: Build ink! contracts @@ -398,7 +398,7 @@ jobs: - name: Start substrate run: echo id=$(docker run -d -p 9944:9944 ghcr.io/hyperledger/solang-substrate-ci:054bef6 substrate-contracts-node --dev --rpc-external) >> $GITHUB_OUTPUT id: substrate - - uses: actions/download-artifact@master + - uses: actions/download-artifact@v3 with: name: solang-linux-x86-64 path: bin @@ -497,32 +497,32 @@ jobs: - name: Install cargo-llvm-cov uses: taiki-e/install-action@cargo-llvm-cov - name: Download Rust coverage files - uses: actions/download-artifact@master + uses: actions/download-artifact@v3 with: name: rust-tests.tar.gz path: ./target - name: Download Solana coverage files - uses: actions/download-artifact@master + uses: actions/download-artifact@v3 with: name: solana-tests path: ./target - name: Download Polkadot coverage files - uses: actions/download-artifact@master + uses: actions/download-artifact@v3 with: name: polkadot-tests path: ./target - name: Download Polkadot subxt coverage files - uses: actions/download-artifact@master + uses: actions/download-artifact@v3 with: name: polkadot-subxt-tests path: ./target - name: Download Anchor coverage files - uses: actions/download-artifact@master + uses: actions/download-artifact@v3 with: name: anchor-tests path: ./target - name: Download VSCode coverage files - uses: actions/download-artifact@master + uses: actions/download-artifact@v3 with: name: vscode-tests path: ./target diff --git a/docs/examples/abi_encode_call.sol b/docs/examples/abi_encode_call.sol index 29b6e452a..f872cd8fa 100644 --- a/docs/examples/abi_encode_call.sol +++ b/docs/examples/abi_encode_call.sol @@ -1,6 +1,6 @@ contract c { function f1() public { - bytes foo = abi.encodeCall(c.bar, 102, true); + bytes foo = abi.encodeCall(c.bar, (102, true)); } function bar(int256 a, bool b) public {} diff --git a/docs/language/builtins.rst b/docs/language/builtins.rst index 663abdf08..c7a43b7c4 100644 --- a/docs/language/builtins.rst +++ b/docs/language/builtins.rst @@ -293,7 +293,8 @@ abi.encodeCall(function, ...) +++++++++++++++++++++++++++++ ABI encodes the function call to the function which should be specified as ``ContractName.FunctionName``. The arguments -are cast and checked against the function specified as the first argument. +are cast and checked against the function specified as the first argument. The arguments must be in a tuple, e.g. +``(a, b, c)``. If there is a single argument no tuple is required. .. include:: ../examples/abi_encode_call.sol :code: solidity @@ -340,7 +341,7 @@ looks like in a solidity contract: is_contract(address AccountId) returns (bool) +++++++++++++++++++++++++++++++++++++++++++++ -Only available on Polkadot. Checks whether the given address is a contract address. +Only available on Polkadot. Checks whether the given address is a contract address. set_code_hash(uint8[32] hash) returns (uint32) ++++++++++++++++++++++++++++++++++++++++++++++ @@ -351,17 +352,17 @@ A return value of 0 indicates success; a return value of 7 indicates that there .. note:: - This is a low level function. We strongly advise consulting the underlying - `API documentation `_ + This is a low level function. We strongly advise consulting the underlying + `API documentation `_ to obtain a full understanding of its implications. -This functionality is intended to be used for implementing upgradeable contracts. +This functionality is intended to be used for implementing upgradeable contracts. Pitfalls generally applying to writing -`upgradeable contracts `_ +`upgradeable contracts `_ must be considered whenever using this builtin function, most notably: * The contract must safeguard access to this functionality, so that it is only callable by priviledged users. -* The code you are upgrading to must be +* The code you are upgrading to must be `storage compatible `_ with the existing code. * Constructors and any other initializers, including initial storage value definitions, won't be executed. diff --git a/src/codegen/mod.rs b/src/codegen/mod.rs index 7c91e2a73..df6305110 100644 --- a/src/codegen/mod.rs +++ b/src/codegen/mod.rs @@ -1671,7 +1671,7 @@ impl Expression { fn external_function_selector(&self) -> Expression { debug_assert!( - matches!(self.ty(), Type::ExternalFunction { .. }), + matches!(self.ty().deref_any(), Type::ExternalFunction { .. }), "This is not an external function" ); let loc = self.loc(); diff --git a/src/sema/builtin.rs b/src/sema/builtin.rs index 0b912b607..34baeecd3 100644 --- a/src/sema/builtin.rs +++ b/src/sema/builtin.rs @@ -8,9 +8,12 @@ use super::diagnostics::Diagnostics; use super::eval::eval_const_number; use super::expression::{ExprContext, ResolveTo}; use super::symtable::Symtable; -use crate::sema::ast::{RetrieveType, Tag, UserTypeDecl}; -use crate::sema::expression::{function_call::evaluate_argument, resolve_expression::expression}; -use crate::sema::namespace::ResolveTypeContext; +use crate::sema::{ + ast::{RetrieveType, Tag, UserTypeDecl}, + expression::{function_call::evaluate_argument, resolve_expression::expression}, + namespace::ResolveTypeContext, + statements::parameter_list_to_expr_list, +}; use crate::Target; use num_bigint::BigInt; use num_traits::One; @@ -1132,69 +1135,24 @@ pub(super) fn resolve_namespace_call( let mut tys = Vec::new(); let mut broken = false; - match &args[1] { - pt::Expression::List(_, list) => { - for (loc, param) in list { - if let Some(param) = param { - let ty = ns.resolve_type( - context.file_no, - context.contract_no, - ResolveTypeContext::None, - ¶m.ty, - diagnostics, - )?; - - if let Some(storage) = ¶m.storage { - diagnostics.push(Diagnostic::error( - storage.loc(), - format!("storage modifier '{storage}' not allowed"), - )); - broken = true; - } - - if let Some(name) = ¶m.name { - diagnostics.push(Diagnostic::error( - name.loc, - format!("unexpected identifier '{}' in type", name.name), - )); - broken = true; - } - - if ty.is_mapping() || ty.is_recursive(ns) { - diagnostics.push(Diagnostic::error( - *loc, - format!("Invalid type '{}': mappings and recursive types cannot be abi decoded or encoded", ty.to_string(ns))) - ); - broken = true; - } - - tys.push(ty); - } else { - diagnostics.push(Diagnostic::error(*loc, "missing type".to_string())); + for arg in parameter_list_to_expr_list(&args[1], diagnostics)? { + let ty = ns.resolve_type( + context.file_no, + context.contract_no, + ResolveTypeContext::None, + arg.strip_parentheses(), + diagnostics, + )?; - broken = true; - } - } + if ty.is_mapping() || ty.is_recursive(ns) { + diagnostics.push(Diagnostic::error( + *loc, + format!("Invalid type '{}': mappings and recursive types cannot be abi decoded or encoded", ty.to_string(ns)) + )); + broken = true; } - _ => { - let ty = ns.resolve_type( - context.file_no, - context.contract_no, - ResolveTypeContext::None, - args[1].remove_parenthesis(), - diagnostics, - )?; - if ty.is_mapping() || ty.is_recursive(ns) { - diagnostics.push(Diagnostic::error( - *loc, - format!("Invalid type '{}': mappings and recursive types cannot be abi decoded or encoded", ty.to_string(ns)) - )); - broken = true; - } - - tys.push(ty); - } + tys.push(ty); } return if broken { @@ -1248,82 +1206,102 @@ pub(super) fn resolve_namespace_call( } } Builtin::AbiEncodeCall => { + if args.len() != 2 { + diagnostics.push(Diagnostic::error( + *loc, + format!("function expects {} arguments, {} provided", 2, args.len()), + )); + + return Err(()); + } + // first argument is function - if let Some(function) = args_iter.next() { - let function = expression( - function, - context, - ns, - symtable, - diagnostics, - ResolveTo::Unknown, - )?; + let function = expression( + &args[0], + context, + ns, + symtable, + diagnostics, + ResolveTo::Unknown, + )?; - match function.ty() { - Type::ExternalFunction { params, .. } - | Type::InternalFunction { params, .. } => { - resolved_args.push(function); - - if args.len() - 1 != params.len() { - diagnostics.push(Diagnostic::error( - *loc, - format!( - "function takes {} arguments, {} provided", - params.len(), - args.len() - 1 - ), - )); - - return Err(()); - } + let ty = function.ty(); - for (arg_no, arg) in args_iter.enumerate() { - let mut expr = expression( - arg, - context, - ns, - symtable, - diagnostics, - ResolveTo::Type(¶ms[arg_no]), - )?; - - expr = expr.cast(&arg.loc(), ¶ms[arg_no], true, ns, diagnostics)?; - - // A string or hex literal should be encoded as a string - if let Expression::BytesLiteral { .. } = &expr { - expr = - expr.cast(&arg.loc(), &Type::String, true, ns, diagnostics)?; - } - - resolved_args.push(expr); - } + match function.cast(&function.loc(), ty.deref_any(), true, ns, diagnostics)? { + Expression::ExternalFunction { function_no, .. } + | Expression::InternalFunction { function_no, .. } => { + let func = &ns.functions[function_no]; - return Ok(Expression::Builtin { - loc: *loc, - tys: vec![Type::DynamicBytes], - kind: builtin, - args: resolved_args, - }); + if !func.is_public() { + diagnostics.push(Diagnostic::error_with_note( + function.loc(), + "function is not public or external".into(), + func.loc, + format!("definition of {}", func.id.name), + )); } - ty => { - diagnostics.push(Diagnostic::error( + + let params = &func.params; + + resolved_args.push(function); + + let args = parameter_list_to_expr_list(&args[1], diagnostics)?; + + if args.len() != params.len() { + diagnostics.push(Diagnostic::error_with_note( *loc, format!( - "first argument should be function, got '{}'", - ty.to_string(ns) + "function takes {} arguments, {} provided", + params.len(), + args.len() ), + func.loc, + format!("definition of {}", func.id.name), )); return Err(()); } + + for (arg_no, arg) in args.iter().enumerate() { + let ty = ns.functions[function_no].params[arg_no].ty.clone(); + + let mut expr = expression( + arg, + context, + ns, + symtable, + diagnostics, + ResolveTo::Type(&ty), + )?; + + expr = expr.cast(&arg.loc(), &ty, true, ns, diagnostics)?; + + // A string or hex literal should be encoded as a string + if let Expression::BytesLiteral { .. } = &expr { + expr = expr.cast(&arg.loc(), &Type::String, true, ns, diagnostics)?; + } + + resolved_args.push(expr); + } + + return Ok(Expression::Builtin { + loc: *loc, + tys: vec![Type::DynamicBytes], + kind: builtin, + args: resolved_args, + }); } - } else { - diagnostics.push(Diagnostic::error( - *loc, - "least one function argument required".to_string(), - )); + expr => { + diagnostics.push(Diagnostic::error( + *loc, + format!( + "first argument should be function, got '{}'", + expr.ty().to_string(ns) + ), + )); - return Err(()); + return Err(()); + } } } Builtin::AbiEncodeWithSignature => { diff --git a/src/sema/expression/mod.rs b/src/sema/expression/mod.rs index e60f2f0aa..4eddfaf8e 100644 --- a/src/sema/expression/mod.rs +++ b/src/sema/expression/mod.rs @@ -843,17 +843,7 @@ impl Expression { } // Lengthing or shorting a fixed bytes array (Type::Bytes(from_len), Type::Bytes(to_len)) => { - if implicit { - diagnostics.push(Diagnostic::cast_error( - *loc, - format!( - "implicit conversion would truncate from {} to {}", - from.to_string(ns), - to.to_string(ns) - ), - )); - Err(()) - } else if to_len > from_len { + if to_len > from_len { let shift = (to_len - from_len) * 8; Ok(Expression::ShiftLeft { @@ -870,6 +860,16 @@ impl Expression { value: BigInt::from_u8(shift).unwrap(), }), }) + } else if implicit { + diagnostics.push(Diagnostic::cast_error( + *loc, + format!( + "implicit conversion would truncate from {} to {}", + from.to_string(ns), + to.to_string(ns) + ), + )); + Err(()) } else { let shift = (from_len - to_len) * 8; diff --git a/src/sema/namespace.rs b/src/sema/namespace.rs index eaa275839..a6daab20a 100644 --- a/src/sema/namespace.rs +++ b/src/sema/namespace.rs @@ -1443,6 +1443,8 @@ impl Namespace { let mut dimensions = vec![]; loop { + expr = expr.strip_parentheses(); + expr = match expr { pt::Expression::ArraySubscript(_, r, None) => { dimensions.push(None); diff --git a/src/sema/statements.rs b/src/sema/statements.rs index e4a5c132d..eead42e7f 100644 --- a/src/sema/statements.rs +++ b/src/sema/statements.rs @@ -2208,7 +2208,7 @@ pub fn parameter_list_to_expr_list<'a>( assert!(annotation.is_none()); diagnostics.push(Diagnostic::error( name.loc, - "single value expected".to_string(), + format!("unexpected identifier '{}'", name.name), )); broken = true; } diff --git a/tests/contract_testcases/evm/bytes_cast.sol b/tests/contract_testcases/evm/bytes_cast.sol new file mode 100644 index 000000000..3891debfc --- /dev/null +++ b/tests/contract_testcases/evm/bytes_cast.sol @@ -0,0 +1,16 @@ +contract C { + function test1(bytes10 x) public returns (bytes8) { + return x; + } + + function test2(bytes10 x) public returns (bytes10) { + return x; + } + + function test3(bytes10 x) public returns (bytes12) { + return x; + } +} + +// ---- Expect: diagnostics ---- +// error: 3:10-11: implicit conversion would truncate from bytes10 to bytes8 diff --git a/tests/contract_testcases/polkadot/builtins/abi_decode_02.sol b/tests/contract_testcases/polkadot/builtins/abi_decode_02.sol index 086c64e29..adf173abf 100644 --- a/tests/contract_testcases/polkadot/builtins/abi_decode_02.sol +++ b/tests/contract_testcases/polkadot/builtins/abi_decode_02.sol @@ -5,4 +5,4 @@ } } // ---- Expect: diagnostics ---- -// error: 4:52-55: unexpected identifier 'feh' in type +// error: 4:52-55: unexpected identifier 'feh' diff --git a/tests/contract_testcases/polkadot/builtins/abi_decode_03.sol b/tests/contract_testcases/polkadot/builtins/abi_decode_03.sol index 6561a9689..6dae59af3 100644 --- a/tests/contract_testcases/polkadot/builtins/abi_decode_03.sol +++ b/tests/contract_testcases/polkadot/builtins/abi_decode_03.sol @@ -5,4 +5,4 @@ } } // ---- Expect: diagnostics ---- -// error: 4:52: missing type +// error: 4:52: stray comma diff --git a/tests/contract_testcases/solana/call/abi_encode_call.sol b/tests/contract_testcases/solana/call/abi_encode_call.sol index b27fa8306..dfb954593 100644 --- a/tests/contract_testcases/solana/call/abi_encode_call.sol +++ b/tests/contract_testcases/solana/call/abi_encode_call.sol @@ -1,18 +1,37 @@ contract abi_encode_call { + int[2] ints; + function test1() public { - bytes bs = abi.encodeCall(other.foo, 1); + bytes memory bs = abi.encodeCall(other.foo, 1); } function test2() public { - bytes bs = abi.encodeCall(other.foo, 1, true); + bytes memory bs = abi.encodeCall(other.foo, (1, true)); + } + + function test3() public { + bytes memory bs = abi.encodeCall(other.baz, (1, 2, ints)); + } + + function test4() public { + bytes memory bs = abi.encodeCall(other.foo, (1, 2), 3); } } contract other { function foo(int foo, int bar) public {} + + function baz(int foo, int bar, int[2] storage) internal {} } + // ---- Expect: diagnostics ---- -// error: 3:20-48: function takes 2 arguments, 1 provided -// error: 7:49-53: conversion from bool to int256 not possible -// warning: 12:22-25: declaration of 'foo' shadows function -// note 12:14-17: previous declaration of function +// error: 5:27-55: function takes 2 arguments, 1 provided +// note 22:5-45: definition of foo +// error: 9:57-61: conversion from bool to int256 not possible +// error: 13:42-47: function is not public or external +// note 24:5-63: definition of baz +// error: 17:27-63: function expects 2 arguments, 3 provided +// warning: 22:22-25: declaration of 'foo' shadows function +// note 22:14-17: previous declaration of function +// warning: 24:22-25: declaration of 'foo' shadows function +// note 22:14-17: previous declaration of function diff --git a/tests/evm.rs b/tests/evm.rs index ae50eb3d4..77bbe1547 100644 --- a/tests/evm.rs +++ b/tests/evm.rs @@ -255,7 +255,7 @@ fn ethereum_solidity_tests() { }) .sum(); - assert_eq!(errors, 909); + assert_eq!(errors, 897); } fn set_file_contents(source: &str, path: &Path) -> (FileResolver, Vec) { diff --git a/tests/solana_tests/call.rs b/tests/solana_tests/call.rs index 3c9c6db88..b663c66ef 100644 --- a/tests/solana_tests/call.rs +++ b/tests/solana_tests/call.rs @@ -322,6 +322,7 @@ fn encode_call() { r#" contract bar0 { bytes8 private constant SELECTOR = bytes8(sha256(bytes('global:test_bar'))); + bytes8 private constant SELECTOR2 = bytes8(sha256(bytes('global:test_baz'))); @account(bar1_pid) function test_other() external returns (int64) { @@ -332,12 +333,25 @@ fn encode_call() { (int64 v) = abi.decode(raw, (int64)); return v + 5; } + + @account(bar1_pid) + function test_other2() external returns (int64) { + bytes select = abi.encodeWithSelector(SELECTOR2, int64(7), int64(5)); + bytes signature = abi.encodeCall(bar1.test_baz, (7, 5)); + require(select == signature, "must be the same"); + (, bytes raw) = tx.accounts.bar1_pid.key.call{accounts: []}(signature); + (int64 v) = abi.decode(raw, (int64)); + return v + 5; + } } contract bar1 { function test_bar(int64 y) public pure returns (int64) { return 3 + y; } + function test_baz(int64 y, int64 x) public pure returns (int64) { + return 3 + y + x; + } }"#, ); @@ -387,6 +401,23 @@ fn encode_call() { value: BigInt::from(15u8) } ); + + let res = vm + .function("test_other2") + .accounts(vec![ + ("bar1_pid", bar1_program_id), + ("systemProgram", [0; 32]), + ]) + .call() + .unwrap(); + + assert_eq!( + res, + BorshToken::Int { + width: 64, + value: BigInt::from(20u8) + } + ); } #[test]