Skip to content

Commit

Permalink
Allow &Env in contract fns (#1210)
Browse files Browse the repository at this point in the history
### What
Allow the first param of contract functions to be &Env or Env, instead
of requiring that if present it is Env.

### Why
To reduce repetitive clones in contract functions.

Requiring as input to functions an owned copy of Env is unnecessary, and
it has a run on affect that if someone wants to use a contract function
also as an internal function called by their code they need to clone the
Env each time it is called. This has zero performance implications
because the Env is, or should be, erased from most places at build time,
but it adds noise to the application. Looking back we should have made
the Env a ref everywhere, probably, but in lieu of doing that which
would be a breaking change we can make it allowed so both are allowed.

This will also remove a compile fail case for devs. Before if they used
an &Env they would see an error. Now they won't.

This change is not a breaking change.
  • Loading branch information
leighmcculloch authored Jan 17, 2024
1 parent eb9f7e2 commit bb38320
Show file tree
Hide file tree
Showing 6 changed files with 211 additions and 10 deletions.
9 changes: 6 additions & 3 deletions soroban-sdk-macros/src/derive_client.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use proc_macro2::TokenStream;
use quote::{format_ident, quote};
use syn::{spanned::Spanned, Error, FnArg, Path, Type, TypePath};
use syn::{spanned::Spanned, Error, FnArg, Path, Type, TypePath, TypeReference};

use crate::syn_ext;

Expand Down Expand Up @@ -141,14 +141,17 @@ pub fn derive_client_impl(crate_path: &Path, name: &str, fns: &[syn_ext::Fn]) ->
// Check for the Env argument.
let env_input = f.inputs.first().and_then(|a| match a {
FnArg::Typed(pat_type) => {
let ty = &*pat_type.ty;
let mut ty = &*pat_type.ty;
if let Type::Reference(TypeReference { elem, .. }) = ty {
ty = elem;
}
if let Type::Path(TypePath {
path: syn::Path { segments, .. },
..
}) = ty
{
if segments.last().map_or(false, |s| s.ident == "Env") {
Some(a)
Some(())
} else {
None
}
Expand Down
19 changes: 14 additions & 5 deletions soroban-sdk-macros/src/derive_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use syn::{
punctuated::Punctuated,
spanned::Spanned,
token::{Colon, Comma},
Attribute, Error, FnArg, Ident, Pat, PatIdent, PatType, Path, Type, TypePath,
Attribute, Error, FnArg, Ident, Pat, PatIdent, PatType, Path, Type, TypePath, TypeReference,
};

#[allow(clippy::too_many_arguments)]
Expand All @@ -25,14 +25,19 @@ pub fn derive_fn(
// Prepare the env input.
let env_input = inputs.first().and_then(|a| match a {
FnArg::Typed(pat_type) => {
let ty = &*pat_type.ty;
let mut is_ref = false;
let mut ty = &*pat_type.ty;
if let Type::Reference(TypeReference { elem, .. }) = ty {
is_ref = true;
ty = elem;
}
if let Type::Path(TypePath {
path: syn::Path { segments, .. },
..
}) = ty
{
if segments.last().map_or(false, |s| s.ident == "Env") {
Some(a)
Some(is_ref)
} else {
None
}
Expand Down Expand Up @@ -87,8 +92,12 @@ pub fn derive_fn(
"use `{}::new(&env, &contract_id).{}` instead",
client_ident, &ident
);
let env_call = if env_input.is_some() {
quote! { env.clone(), }
let env_call = if let Some(is_ref) = env_input {
if is_ref {
quote! { &env, }
} else {
quote! { env.clone(), }
}
} else {
quote! {}
};
Expand Down
8 changes: 6 additions & 2 deletions soroban-sdk-macros/src/derive_spec_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use stellar_xdr::{
ScSpecEntry, ScSpecFunctionInputV0, ScSpecFunctionV0, ScSpecTypeDef, ScSymbol, StringM, VecM,
WriteXdr, SCSYMBOL_LIMIT,
};
use syn::TypeReference;
use syn::{
punctuated::Punctuated, spanned::Spanned, token::Comma, Attribute, Error, FnArg, Ident, Pat,
ReturnType, Type, TypePath,
Expand All @@ -27,14 +28,17 @@ pub fn derive_fn_spec(
// Prepare the env input.
let env_input = inputs.first().and_then(|a| match a {
FnArg::Typed(pat_type) => {
let ty = &*pat_type.ty;
let mut ty = &*pat_type.ty;
if let Type::Reference(TypeReference { elem, .. }) = ty {
ty = elem;
}
if let Type::Path(TypePath {
path: syn::Path { segments, .. },
..
}) = ty
{
if segments.last().map_or(false, |s| s.ident == "Env") {
Some(a)
Some(())
} else {
None
}
Expand Down
1 change: 1 addition & 0 deletions soroban-sdk/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ mod contract_add_i32;
mod contract_assert;
mod contract_docs;
mod contract_duration;
mod contract_fn;
mod contract_invoke;
mod contract_overlapping_type_fn_names;
mod contract_snapshot;
Expand Down
52 changes: 52 additions & 0 deletions soroban-sdk/src/tests/contract_fn.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
use crate as soroban_sdk;
use soroban_sdk::{contract, contractimpl, Env};
use stellar_xdr::curr as stellar_xdr;
use stellar_xdr::{
Limits, ReadXdr, ScSpecEntry, ScSpecFunctionInputV0, ScSpecFunctionV0, ScSpecTypeDef,
};

#[contract]
pub struct Contract;

#[contractimpl]
impl Contract {
pub fn add(_e: &Env, a: i32, b: i32) -> i32 {
a + b
}
}

#[test]
fn test_functional() {
let e = Env::default();
let contract_id = e.register_contract(None, Contract);

let a = 10i32;
let b = 12i32;
let c = ContractClient::new(&e, &contract_id).add(&a, &b);
assert_eq!(c, 22);
}

#[test]
fn test_spec() {
let entries = ScSpecEntry::from_xdr(__SPEC_XDR_FN_ADD, Limits::none()).unwrap();
let expect = ScSpecEntry::FunctionV0(ScSpecFunctionV0 {
doc: "".try_into().unwrap(),
name: "add".try_into().unwrap(),
inputs: vec![
ScSpecFunctionInputV0 {
doc: "".try_into().unwrap(),
name: "a".try_into().unwrap(),
type_: ScSpecTypeDef::I32,
},
ScSpecFunctionInputV0 {
doc: "".try_into().unwrap(),
name: "b".try_into().unwrap(),
type_: ScSpecTypeDef::I32,
},
]
.try_into()
.unwrap(),
outputs: vec![ScSpecTypeDef::I32].try_into().unwrap(),
});
assert_eq!(entries, expect);
}
132 changes: 132 additions & 0 deletions soroban-sdk/test_snapshots/tests/contract_fn/test_functional.1.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
{
"generators": {
"address": 1,
"nonce": 0
},
"auth": [
[]
],
"ledger": {
"protocol_version": 20,
"sequence_number": 0,
"timestamp": 0,
"network_id": "0000000000000000000000000000000000000000000000000000000000000000",
"base_reserve": 0,
"min_persistent_entry_ttl": 4096,
"min_temp_entry_ttl": 16,
"max_entry_ttl": 6312000,
"ledger_entries": [
[
{
"contract_data": {
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM",
"key": "ledger_key_contract_instance",
"durability": "persistent"
}
},
[
{
"last_modified_ledger_seq": 0,
"data": {
"contract_data": {
"ext": "v0",
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM",
"key": "ledger_key_contract_instance",
"durability": "persistent",
"val": {
"contract_instance": {
"executable": {
"wasm": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
},
"storage": null
}
}
}
},
"ext": "v0"
},
4095
]
],
[
{
"contract_code": {
"hash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
}
},
[
{
"last_modified_ledger_seq": 0,
"data": {
"contract_code": {
"ext": "v0",
"hash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
"code": ""
}
},
"ext": "v0"
},
4095
]
]
]
},
"events": [
{
"event": {
"ext": "v0",
"contract_id": null,
"type_": "diagnostic",
"body": {
"v0": {
"topics": [
{
"symbol": "fn_call"
},
{
"bytes": "0000000000000000000000000000000000000000000000000000000000000001"
},
{
"symbol": "add"
}
],
"data": {
"vec": [
{
"i32": 10
},
{
"i32": 12
}
]
}
}
}
},
"failed_call": false
},
{
"event": {
"ext": "v0",
"contract_id": "0000000000000000000000000000000000000000000000000000000000000001",
"type_": "diagnostic",
"body": {
"v0": {
"topics": [
{
"symbol": "fn_return"
},
{
"symbol": "add"
}
],
"data": {
"i32": 22
}
}
}
},
"failed_call": false
}
]
}

0 comments on commit bb38320

Please sign in to comment.