Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sol!): function calls should directly yield result #855

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

yash-atreya
Copy link
Member

@yash-atreya yash-atreya commented Jan 14, 2025

Motivation

Currently, when working with named or unnamed return parameters like the following

sol! {
    function named() returns (uint256 a);
    function unnamed() returns (uint256);
}

the user has to do namedReturn.a or unnamedReturn._0 to access the actual result i.e calling the function does not directly yield the result. This is cumbersome and becomes even more so when working with structs, or multiple return values etc.

Solution

  1. Change type Return in the impl SolCall of the call type.
  2. Map the return type from the wrapped type to the inner one (_0) in abi_decode_returns to return directly

Consider the following

sol! {
    function balanceOf(address owner) returns (uint256);
}

The bindings generated by the above have changed like so:

impl SolCall for balanceOfCall {
// ....
    type Return = alloy_primitives::U256 // Previously, balanceOfReturn { _0: U256 }
// ....
    fn abi_decode_returns(data: &[u8], validate: bool) -> Result<Self::Return> {
         abi_decode_sequence(data, validate).map(|r: balanceOfReturn| r._0)  
    }
}

This not only works for unamed singular return types but also named ones and tuple/compound return types. E.g

sol! {
    function balanceOf(address owner) returns (uint256 bal);
    function balanceAndBlockNumber(address owner) returns (uint256 bal, uint64)
}
// ... 
// Yields U256. previously yielded `balanceOfReturn { bal: U256 }` 
let bal = balanceOfCall::abi_decode_returns(&data, validate).unwrap(); 

// Yields (U256, u64), previously yielded `balanceAndBlockReturn { bal: U256, _1: u64 }`
let (bal, block_num) = balanceAndBlockCall::abi_decode_returns(&data, validate).unwrap(); 

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@yash-atreya yash-atreya changed the title feat(sol!): functions should directly yield result feat(sol!): function should directly yield result Jan 14, 2025
@yash-atreya yash-atreya changed the title feat(sol!): function should directly yield result feat(sol!): function calls should directly yield result Jan 14, 2025
@yash-atreya yash-atreya marked this pull request as ready for review January 14, 2025 14:03
@yash-atreya yash-atreya marked this pull request as draft January 15, 2025 08:04
@yash-atreya yash-atreya marked this pull request as ready for review January 15, 2025 09:07
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ptal @DaniPopes

Comment on lines 128 to 130
_ => {
let return_tys = expand_types(returns, cx);
let tuple_ret = generate_return_tuple(returns);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did we handle tuples previously? as this also _0: (..,..,..)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sol! {
   function simple() returns (uint256, uint64);
}

Previously, we handled it by creating a field for each return param in the return struct. For above, it would be

struct simpleReturn {
_0: U256,
_1: u64
}

Now we return (simpleReturn._0, simpleReturn._1) for simple.call() instead of the struct.

crates/sol-macro-expander/src/expand/mod.rs Outdated Show resolved Hide resolved
crates/sol-macro-expander/src/expand/function.rs Outdated Show resolved Hide resolved
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of this change, however we're basically throwing out the API of the return struct and so we lose the names of the return variables if present.

How about we generate the tuple return if ALL return parameters are unnamed, otherwise generate and use the return struct?

We also need to document this on the macro doccoments, and I would like to see a PR with a patch on the examples repo to see this in action before merging this.

Edit: logic of the question.

crates/sol-types/tests/macros/sol/mod.rs Show resolved Hide resolved
@yash-atreya
Copy link
Member Author

Examples patch: alloy-rs/examples#174

@yash-atreya
Copy link
Member Author

yash-atreya commented Jan 17, 2025

How about we generate the tuple return if ALL return parameters are unnamed, otherwise generate and use the return struct?

I started with this initially but I feel the devX gets cumbersome as we still retain the _0, _1.. fields in the struct. Moreover, if we have return statement where one param is named and others are not, it gets worse.

E.g.

sol! {
function getPartialNamedTuple() external view returns (uint256 , uint256 b, uint256 );
}
// Gives the following return struct: 
struct getPartialNamedTupleReturn {
    _0: U256,
    b: U256,
    _2: U256
}

I'm fine with losing names if we have behavior where return types are easily pattern-matched. i.e If a solidity func returns multiple parameters akin to a tuple, then the rust binding shouldn't return a struct with the return params as fields,it should return the tuple directly. Having said that, open to other ideas. wdyt? @DaniPopes @mattsse @gakonst

We can add extensive docs to explain the above

You can find an example with more complex return types here (compared to previous <name>Return struct version): https://github.com/alloy-rs/examples/blob/46372dafc30c281afab2cb0c994372c8011b220c/examples/sol-macro/examples/complex_returns.rs

@mattsse
Copy link
Member

mattsse commented Jan 17, 2025

this has benefits when you're only interested in one field or always want to decompose the tuple right away.

imo this has significant downsides when you want to do some processing on the return object

fn handle_return(x: Return {a,b,c})

vs

fn handle_return(x: (U256, String, U256))

without the type and field names you lose the entire context right away.
the tuple type could be provided as an alias but this doesn't seem like an improvement to me

This reverts commit df62cc9.
This reverts commit 2c39af0.
@yash-atreya
Copy link
Member Author

False positive in clippy rust-lang/rust-clippy#14020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants