[WIP]lang: Remove program info from CpiContext

[paul-schaaf]

closes #522

[paul-schaaf]

@armaniferrante this is now almost done. There are some instances where I didn't know what to do which I have marked with TODO[paulx]. They're all connected to the #[interface] attribute. Would you mind taking a look?

[archseer]

Why was shmem removed?

[paul-schaaf]

the change broke the shmem program and armani told me (offline) that instead of fixing it I can just delete it cause it's not used by anyone. In addition, solana will allow for CPIs to return values soon so this program wont be needed anymore.

[armaniferrante]

We should remove it.

[armaniferrante]

What's the problem here? This seems fine, though I prefer moving the cpi_ctx above where it previously was.

[paul-schaaf]

yea I think the todo in the is_realized function is the only one that actually matters. There the program comes from a remaining account instead of the Accounts struct so wasn't sure whether I can just add it as a proper field to the Account struct without breaking something because Im not super familiar yet with the #[access_control] and #[interface] attributes

also sure, I'll move cpi_ctx in back to where it was. I actually did this in a lot of places. My way of thinking about this has been that moving things into variables if they are not a) mutated or b) used at least twice is not a good idea because variables imply to the reader that either a) or b) is going to happen and it confuses the reader if neither happens (with the exception where a function call or struct becomes too confusing to read). curious what u think about that.

[armaniferrante]

So we actually need the cpi_program variable for #[interface] because the interface CPI "client" doesn't actually know what program ID to use, so it needs to be provided. See https://github.com/project-serum/anchor/blob/master/lang/attribute/interface/src/lib.rs#L216

For all other CPI's, however, this is not the case, since the program address is hardcoded in the CPI client for security reasons (facilitated with the use of declare_id)

So our options are

1. Keep The old API with CpiContext::new(cpi_program, cpi_accounts) and only use cpi_program with the #[interface] client.
2. Use the new api, and make cpi_program an option inside CpiContext, e.g., CpiContext::new(cpi_accounts).with_program(cpi_program).

What do you think?

[paul-schaaf]

3..) for interface cpi calls, add a program arg after the cpi_context and before the args expansion in the function signature

1..) if we keep the old api, wouldnt all calls use cpi_program, not just interface ones? im probably misunderstanding

2..) I like this too

[armaniferrante]

1. No because the cpi clients hardcode the program_id when constructing the Instruction struct for CPI. This is done via the crate::ID variable defined by declare_id in all Anchor programs.

I'm not a big fan of 3). But am happy with both 1 and 2. Maybe we go with 1) for now to avoid the breaking change.

[paul-schaaf]

1. ah what I meant is that callers would still use cpi_program in all programs to pass it into new (even tho it is not used for the actual call)

2. seems ok to me, better break things now than later

[paul-schaaf]

if we go with 2 maybe instead of an option, there could be a new type InterfaceCpiContext. for normal cpi calls u should not be able to change the program whereas for interface calls you always need to

[armaniferrante]

@fanatid thoughts?

[fanatid]

I do not have strong thoughts about Cpi/interface 😐

[armaniferrante]

I'd prefer to not introduce a new type, avoid the breaking change, and revisit this once we address #454, which will determine the future of #[interface].

[armaniferrante]

@armaniferrante this is now almost done. There are some instances where I didn't know what to do which I have marked with TODO[paulx]. They're all connected to the #[interface] attribute. Would you mind taking a look?

Commented on all the remaining TODOs.

[Henry-E]

It unfortunate this was never merged. Given that the main blocker #[interface] was removed I think we can look at maybe trying to make this change again.

From the sounds of it the advantages are

• less memory usage because there's one less account info being passed around
• in all of the CPI clients the program ID of the program being invoked is hardcoded anyway, the only purpose of passing the account info of the target program was a requirement by the solana runtime which has apparently gone away.

Cons are

• it looks like it's a breaking change for lots of people probably, not sure how much of a consideration this is for the efficiency gains these days?

Also it looks like shmem is still in the anchor spl library even though we now have return values from CPI. I don't know too much about that but i would have thought shmem would have just been removed in a separate PR.

[armaniferrante]

Closing as stale.