Skip to content

Commit

Permalink
Add #[payable] annotation to ABI methods (FuelLabs#3450)
Browse files Browse the repository at this point in the history
## Description

The lack of this annotation implies the method is non-payable. When
calling an ABI method that is non-payable, the compiler must emit an
error if the amount of coins to forward is not guaranteed to be zero.

The compiler also emits an error if the method signature and the method
implementation have different `#[payable]` annotations.

## Assumptions

Currently, the analysis of non-zero coins is very simple and only checks
if the special `coins:` contract call parameter is the zero `u64`
literal directly or can be reached through a chain of variable/constant
definitions.

## Tests

- [x] Must fail when scripts call non-payable methods with non-zero
coins as a literal
- [x] Must fail when scripts call non-payable methods with non-zero
coins as a constant
- [x] Must fail when scripts call non-payable methods with non-zero
coins as a variable definition
- [x] Must fail when contracts call non-payable methods with non-zero
coins
- [x] Must fail when there is an extra `#[payable]` annotation in method
implementation (not mentioned in its signature)
- [x] Must fail when the `#[payable]` annotation in method
implementation is missing (but mentioned in its signature)

close FuelLabs#1608

## TODOs

- [x] Fix `#[payable]` annotations for the standard library
- [x] Fix the empty parens issue for formatting attributes: FuelLabs#3451
- [x] Parser should allow us write annotations like so: `#[storage(read,
write), payable]`: FuelLabs#3452
- [x] Refactor `#[payable]` annotations in this PR in the style above
- [x] Add more complex formatter unit tests with `payable`
- [x] As pointed out by @mohammadfawaz, the SDK can bypass payable
annotations and one option would be to add function attributes to the
[JSON ABI
schema](https://fuellabs.github.io/fuel-specs/master/protocol/abi/json_abi_format.html)
and process it in the SDK (see these issues:
FuelLabs/fuel-specs#446 and
FuelLabs/fuels-rs#742)
- [x] Bump `fuels-rs`'s version

Co-authored-by: Mohammad Fawaz <[email protected]>
  • Loading branch information
anton-trunov and mohammadfawaz authored Dec 19, 2022
1 parent 02d9b3d commit 211667c
Show file tree
Hide file tree
Showing 282 changed files with 1,683 additions and 19 deletions.
12 changes: 6 additions & 6 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions examples/cei_analysis/src/other_contract.sw
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
library other_contract;

abi OtherContract {
#[payable]
fn external_call();
}
2 changes: 1 addition & 1 deletion forc-pkg/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ anyhow = "1"
fd-lock = "3.0"
forc-tracing = { version = "0.32.2", path = "../forc-tracing" }
forc-util = { version = "0.32.2", path = "../forc-util" }
fuels-types = "0.32"
fuels-types = "0.33"
git2 = { version = "0.14", features = ["vendored-libgit2", "vendored-openssl"] }
hex = "0.4.3"
petgraph = { version = "0.6", features = ["serde-1"] }
Expand Down
6 changes: 3 additions & 3 deletions forc-plugins/forc-client/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ forc-tracing = { version = "0.32.2", path = "../../forc-tracing" }
forc-util = { version = "0.32.2", path = "../../forc-util" }
fuel-gql-client = { version = "0.15", default-features = false }
fuel-tx = { version = "0.23", features = ["builder"] }
fuels-core = "0.32"
fuels-signers = "0.32"
fuels-types = "0.32"
fuels-core = "0.33"
fuels-signers = "0.33"
fuels-types = "0.33"
futures = "0.3"
hex = "0.4.3"
serde = "1.0"
Expand Down
2 changes: 1 addition & 1 deletion sway-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ derivative = "2.2.0"
dirs = "3.0"
either = "1.6"
fuel-vm = { version = "0.22", features = ["serde"] }
fuels-types = "0.32"
fuels-types = "0.33"
hashbrown = "0.13.1"
hex = { version = "0.4", optional = true }
im = "15.0"
Expand Down
1 change: 1 addition & 0 deletions sway-core/src/language/ty/declaration/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,7 @@ impl TyFunctionDeclaration {
self.return_type,
),
},
attributes: transform::generate_json_abi_attributes_map(&self.attributes),
}
}

Expand Down
1 change: 1 addition & 0 deletions sway-core/src/semantic_analysis.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! Type checking for Sway.
pub mod ast_node;
pub(crate) mod cei_pattern_analysis;
pub(crate) mod coins_analysis;
mod module;
pub mod namespace;
mod node_dependencies;
Expand Down
29 changes: 29 additions & 0 deletions sway-core/src/semantic_analysis/ast_node/declaration/impl_trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,35 @@ fn type_check_trait_implementation(
});
}

// check there is no mismatch of payability attributes
// between the method signature and the method implementation
use crate::transform::AttributeKind::Payable;
let impl_method_signature_payable = impl_method_signature.attributes.contains_key(&Payable);
let impl_method_payable = impl_method.attributes.contains_key(&Payable);
match (impl_method_signature_payable, impl_method_payable) {
(true, false) =>
// implementation does not have payable attribute
{
errors.push(CompileError::TraitImplPayabilityMismatch {
fn_name: impl_method.name.clone(),
interface_name: interface_name(),
missing_impl_attribute: true,
span: impl_method.span.clone(),
})
}
(false, true) =>
// implementation has extra payable attribute, not mentioned by signature
{
errors.push(CompileError::TraitImplPayabilityMismatch {
fn_name: impl_method.name.clone(),
interface_name: interface_name(),
missing_impl_attribute: false,
span: impl_method.span.clone(),
})
}
(true, true) | (false, false) => (), // no payability mismatch
}

impl_method
.return_type
.replace_self_type(type_engine, self_type);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ use crate::{
use ast_node::typed_expression::check_function_arguments_arity;
use std::collections::{HashMap, VecDeque};
use sway_error::error::CompileError;
use sway_types::Spanned;
use sway_types::{constants, integer_bits::IntegerBits};
use sway_types::{constants::CONTRACT_CALL_COINS_PARAMETER_NAME, Spanned};
use sway_types::{state::StateIndex, Span};

#[allow(clippy::too_many_arguments)]
Expand Down Expand Up @@ -139,6 +139,24 @@ pub(crate) fn type_check_method_application(
}
};
}

// check if method is non-payable but we do not know _statically_
// the amount of coins sent in the contract call is zero
// if the coins contract call parameter is not specified
// it's considered to be zero and hence no error needs to be reported
if let Some(coins_expr) = contract_call_params_map.get(CONTRACT_CALL_COINS_PARAMETER_NAME) {
if coins_analysis::possibly_nonzero_u64_expression(ctx.namespace, coins_expr)
&& !method
.attributes
.contains_key(&crate::transform::AttributeKind::Payable)
{
errors.push(CompileError::CoinsPassedToNonPayableMethod {
fn_name: method.name,
span,
});
return err(warnings, errors);
}
}
}

// If this method was called with self being a `StorageAccess` (e.g. storage.map.insert(..)),
Expand Down
77 changes: 77 additions & 0 deletions sway-core/src/semantic_analysis/coins_analysis.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
use crate::{
language::ty::{self, TyExpression},
Namespace,
};

// This analysis checks if an expression is known statically to evaluate
// to a non-zero value at runtime.
// It's intended to be used in the payability analysis to check if a non-payable
// method gets called with a non-zero amount of `coins`
pub fn possibly_nonzero_u64_expression(namespace: &Namespace, expr: &TyExpression) -> bool {
use ty::TyExpressionVariant::*;
match &expr.expression {
Literal(crate::language::Literal::U64(value)) => *value != 0,
// not a u64 literal, hence we return true to be on the safe side
Literal(_) => true,
VariableExpression { name, .. } => {
match namespace.resolve_symbol(name).value {
Some(ty_decl) => {
match ty_decl {
ty::TyDeclaration::VariableDeclaration(var_decl) => {
possibly_nonzero_u64_expression(namespace, &var_decl.body)
}
ty::TyDeclaration::ConstantDeclaration(decl_id) => {
match crate::declaration_engine::de_get_constant(
decl_id.clone(),
&expr.span,
) {
Ok(const_decl) => {
possibly_nonzero_u64_expression(namespace, &const_decl.value)
}
Err(_) => true,
}
}
_ => true, // impossible cases, true is a safer option here
}
}
None => {
// Unknown variable, but it's not possible in a well-typed expression
// returning true here just to be on the safe side
true
}
}
}
// We do not treat complex expressions at the moment: the rational for this
// is that the `coins` contract call parameter is usually a literal, a variable,
// or a constant.
// Since we don't analyze the following types of expressions, we just assume
// those result in non-zero amount of coins
FunctionApplication { .. }
| ArrayIndex { .. }
| CodeBlock(_)
| IfExp { .. }
| AsmExpression { .. }
| StructFieldAccess { .. }
| TupleElemAccess { .. }
| StorageAccess(_)
| WhileLoop { .. } => true,
// The following expression variants are unreachable, because of the type system
// but we still consider these as non-zero to be on the safe side
LazyOperator { .. }
| Tuple { .. }
| Array { .. }
| StructExpression { .. }
| FunctionParameter
| EnumInstantiation { .. }
| AbiCast { .. }
| IntrinsicFunction(_)
| AbiName(_)
| UnsafeDowncast { .. }
| EnumTag { .. }
| Break
| Continue
| Reassignment(_)
| Return(_)
| StorageReassignment(_) => true,
}
}
21 changes: 21 additions & 0 deletions sway-core/src/transform/attribute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,28 @@ pub enum AttributeKind {
Storage,
Inline,
Test,
Payable,
}

/// Stores the attributes associated with the type.
pub type AttributesMap = Arc<HashMap<AttributeKind, Vec<Attribute>>>;

pub(crate) fn generate_json_abi_attributes_map(
attr_map: &AttributesMap,
) -> Option<Vec<fuels_types::Attribute>> {
if attr_map.is_empty() {
None
} else {
Some(
attr_map
.iter()
.flat_map(|(_attr_kind, attrs)| {
attrs.iter().map(|attr| fuels_types::Attribute {
name: attr.name.to_string(),
arguments: attr.args.iter().map(|arg| arg.to_string()).collect(),
})
})
.collect(),
)
}
}
6 changes: 4 additions & 2 deletions sway-core/src/transform/to_parsed_lang/convert_parse_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@ use sway_error::warning::{CompileWarning, Warning};
use sway_types::{
constants::{
DESTRUCTURE_PREFIX, DOC_ATTRIBUTE_NAME, DOC_COMMENT_ATTRIBUTE_NAME, INLINE_ATTRIBUTE_NAME,
MATCH_RETURN_VAR_NAME_PREFIX, STORAGE_PURITY_ATTRIBUTE_NAME, STORAGE_PURITY_READ_NAME,
STORAGE_PURITY_WRITE_NAME, TEST_ATTRIBUTE_NAME, TUPLE_NAME_PREFIX, VALID_ATTRIBUTE_NAMES,
MATCH_RETURN_VAR_NAME_PREFIX, PAYABLE_ATTRIBUTE_NAME, STORAGE_PURITY_ATTRIBUTE_NAME,
STORAGE_PURITY_READ_NAME, STORAGE_PURITY_WRITE_NAME, TEST_ATTRIBUTE_NAME,
TUPLE_NAME_PREFIX, VALID_ATTRIBUTE_NAMES,
},
integer_bits::IntegerBits,
};
Expand Down Expand Up @@ -3283,6 +3284,7 @@ fn item_attrs_to_map(
STORAGE_PURITY_ATTRIBUTE_NAME => Some(AttributeKind::Storage),
INLINE_ATTRIBUTE_NAME => Some(AttributeKind::Inline),
TEST_ATTRIBUTE_NAME => Some(AttributeKind::Test),
PAYABLE_ATTRIBUTE_NAME => Some(AttributeKind::Payable),
_ => None,
} {
match attrs_map.get_mut(&attr_kind) {
Expand Down
19 changes: 19 additions & 0 deletions sway-error/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -694,6 +694,23 @@ pub enum CompileError {
CallingPrivateLibraryMethod { name: String, span: Span },
#[error("Using \"while\" in a predicate is not allowed.")]
DisallowedWhileInPredicate { span: Span },
#[error("Possibly non-zero amount of coins transferred to non-payable contract method \"{fn_name}\".")]
CoinsPassedToNonPayableMethod { fn_name: Ident, span: Span },
#[error(
"Payable attribute mismatch. The \"{fn_name}\" method implementation \
{} in its signature in {interface_name}.",
if *missing_impl_attribute {
"is missing #[payable] attribute specified"
} else {
"has extra #[payable] attribute not mentioned"
}
)]
TraitImplPayabilityMismatch {
fn_name: Ident,
interface_name: InterfaceName,
missing_impl_attribute: bool,
span: Span,
},
}

impl std::convert::From<TypeError> for CompileError {
Expand Down Expand Up @@ -879,6 +896,8 @@ impl Spanned for CompileError {
DisallowedControlFlowInstruction { span, .. } => span.clone(),
CallingPrivateLibraryMethod { span, .. } => span.clone(),
DisallowedWhileInPredicate { span } => span.clone(),
CoinsPassedToNonPayableMethod { span, .. } => span.clone(),
TraitImplPayabilityMismatch { span, .. } => span.clone(),
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions sway-types/src/constants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,15 @@ pub const DOC_COMMENT_ATTRIBUTE_NAME: &str = "doc-comment";
/// The attribute used for Sway in-language unit tests.
pub const TEST_ATTRIBUTE_NAME: &str = "test";

/// The valid attribute string used for payable functions.
pub const PAYABLE_ATTRIBUTE_NAME: &str = "payable";

/// The list of valid attributes.
pub const VALID_ATTRIBUTE_NAMES: &[&str] = &[
STORAGE_PURITY_ATTRIBUTE_NAME,
DOC_ATTRIBUTE_NAME,
DOC_COMMENT_ATTRIBUTE_NAME,
TEST_ATTRIBUTE_NAME,
INLINE_ATTRIBUTE_NAME,
PAYABLE_ATTRIBUTE_NAME,
];
Loading

0 comments on commit 211667c

Please sign in to comment.