Skip to content

Commit

Permalink
Fix abi.encodeCall() argument parsing (#1612)
Browse files Browse the repository at this point in the history 
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 <sean@mess.org>
  • Loading branch information
@seanyoung
seanyoung committed Dec 20, 2023
1 parent 92a6c6c commit 358a184
Show file tree
Hide file tree
Showing 14 changed files with 212 additions and 165 deletions.
20 changes: 10 additions & 10 deletions .github/workflows/test.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,16 +356,16 @@ 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
path: bin
- 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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion docs/examples/abi_encode_call.sol
View file
Original file line number Diff line number Diff line change
@@ -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 {}
Expand Down
15 changes: 8 additions & 7 deletions docs/language/builtins.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
++++++++++++++++++++++++++++++++++++++++++++++
Expand All @@ -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 <https://docs.rs/pallet-contracts/latest/pallet_contracts/api_doc/trait.Version0.html#tymethod.set_code_hash>`_
This is a low level function. We strongly advise consulting the underlying
`API documentation <https://docs.rs/pallet-contracts/latest/pallet_contracts/api_doc/trait.Version0.html#tymethod.set_code_hash>`_
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 <https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable>`_
`upgradeable contracts <https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable>`_
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 <https://docs.openzeppelin.com/upgrades-plugins/1.x/proxies#storage-collisions-between-implementation-versions>`_
with the existing code.
* Constructors and any other initializers, including initial storage value definitions, won't be executed.
Expand Down
2 changes: 1 addition & 1 deletion src/codegen/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
228 changes: 103 additions & 125 deletions src/sema/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
&param.ty,
diagnostics,
)?;

if let Some(storage) = &param.storage {
diagnostics.push(Diagnostic::error(
storage.loc(),
format!("storage modifier '{storage}' not allowed"),
));
broken = true;
}

if let Some(name) = &param.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 {
Expand Down Expand Up @@ -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(&params[arg_no]),
)?;

expr = expr.cast(&arg.loc(), &params[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 => {
Expand Down
Loading

0 comments on commit 358a184

Please sign in to comment.