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

lang: Allow CPI return values #1598

Merged
merged 38 commits into from
Mar 24, 2022

Conversation

tomlinton
Copy link
Contributor

If an instruction returns a type then write the result with the Solana set_return_data syscall. If the CPI client determines an instruction returns something it'll read it with get_return_data.

This shouldn't change anything for standard Result<()> returns from instructions, but specifying a Result with a type will result in a set_return_data call regardless of whether the call is made via CPI. I haven't figured out whether that is a good thing or a bad thing.

Closes #20

@tomlinton tomlinton marked this pull request as ready for review March 11, 2022 20:23
Copy link
Contributor

@paul-schaaf paul-schaaf left a comment

Choose a reason for hiding this comment

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

the generated idl should also include the return type of the instruction

tests/cpi-returns/yarn.lock Outdated Show resolved Hide resolved
lang/syn/src/codegen/program/handlers.rs Outdated Show resolved Hide resolved
tests/cpi-returns/programs/caller/src/lib.rs Outdated Show resolved Hide resolved
tests/cpi-returns/programs/callee/src/lib.rs Show resolved Hide resolved
lang/syn/src/codegen/program/handlers.rs Outdated Show resolved Hide resolved
tests/cpi-returns/programs/callee/src/lib.rs Show resolved Hide resolved
@paul-schaaf
Copy link
Contributor

specifying a Result with a type will result in a set_return_data call regardless of whether the call is made via CPI

seems like a general problem with set_return_data though, not a problem with your implementation. Or am I misunderstanding you?

@tomlinton
Copy link
Contributor Author

the generated idl should also include the return type of the instruction

Added this.

seems like a general problem with set_return_data though, not a problem with your implementation. Or am I misunderstanding you?

Yea I think this is OK.

"()" => (quote! {anchor_lang::Result<()> }, quote! { Ok(()) }),
_ => (
quote! { anchor_lang::Result<Return::<#ret_type>> },
quote! { Ok(Return::<#ret_type> { phantom: PhantomData }) }
Copy link
Contributor

Choose a reason for hiding this comment

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

use full path for Return and PhantomData to avoid naming clashes (in the line above there's a Return too)


pub fn parse_return(method: &syn::ItemFn) -> ParseResult<IxReturn> {
match method.sig.output {
syn::ReturnType::Type(_, ref ty) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

an error here would be nice that hardcodes (with .to_string) that the return type must be of the form Result<()> or Result<T>

the return code (and possibly even other code) depends on this exact return type and you can't use aliases like type MyResult = Result<()>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is still some tests returning ProgramResult, this handles return types that don't confirm to Result<()> or Result<T> by defaulting to assuming it returns the unit type, these might break I think? I can commit it anyway and let CI figure it out

Copy link
Contributor

Choose a reason for hiding this comment

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

yea I cant think of anything that can actually go wrong with your code defaulting to no return type. so no need for that extra check i think

pub fn get(&self) -> Result<T> {
let (_key, data) = anchor_lang::solana_program::program::get_return_data().unwrap();
let value = T::try_from_slice(&data).unwrap();
Ok(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this could just return T instead of Result<T>

@@ -0,0 +1,13 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the way it works / what worked for me is that you dont have to specify the dependencies here. The package.json file in the tests dir already has them and this folder can pull them as long as its defined as a npm workspace member in the package.json in tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea that is right

@paul-schaaf
Copy link
Contributor

LGTM! Is there anything left you want to add or can I merge?

@tomlinton
Copy link
Contributor Author

All good from my end, thanks! 🚀

@paul-schaaf paul-schaaf merged commit 1cb7429 into coral-xyz:master Mar 24, 2022
@paul-schaaf
Copy link
Contributor

thank you for the contribution!

@tomlinton tomlinton mentioned this pull request Mar 28, 2022
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.

CPI client with return values
2 participants