PDAs as token account authorities are throwing an unusual error

[harsh4786]

I tried passing PDAs as token account authorities but I get an unusual error like this

the issue wasn't happening in seahorse v0.1.6

try this out yourself on Solana playground..

https://beta.solpg.io/635d590977ea7f12846aef06

[mcintyre94]

Hmm okay I see what's going on here. First of all, here's a version of your playground that builds and that I think does what you want: https://beta.solpg.io/635e831677ea7f12846aef09

The change I made is that I do static_acc = static_acc.init(...). So there's no second variable introduced, everything is static_acc.

Now this is what it's compiling to (just showing the important parts of the accounts struct for your instruction):

    #[derive(Accounts)]
    # [instruction (noise : u64 , signal : u64 , metadata : String)]
    pub struct InitializeStatic<'info> {
        <snipped stuff>,
        # [account (
          init ,
          space = <snip>,
          payer = payer,
          seeds = <snip> ,
          bump
        )]
        pub static_acc: Box<Account<'info, dot::program::StaticShell>>,
        # [account (
          init ,
          payer = payer ,
          seeds = <snip>,
          bump ,
          token :: mint = mint ,
          token :: authority = static_acc
        )]
        pub token: Box<Account<'info, TokenAccount>>,
    }

In Anchor this structure is independent of the instruction itself. It can receive some data passed into the instruction (like metadata in your seeds is fine), but it can't receive stuff constructed directly in your instruction, like static_account. So not being able to use static_account does make sense, and AFAIK isn't something we can fix. There's no way to pass static_account in here, it's constructed after this account validation stuff.

So then the question is should we allow you to pass static_acc, if Seahorse only ever sees it as an Empty[StaticShell]. That's what's happening in your example, because you're assigning the initialised account to the new variable static_account. So as far as Seahorse is aware when reading your code, static_acc is an empty address and static_address is an initialized one.

IMO the best approach here is to leave Seahorse as-is and require you to explicitly reassign static_acc. But I do appreciate that this is a kinda tricky limitation to explain and is really requiring you to know a bit about the Anchor representation of your code.

My concern is otherwise you could compile code like this:

@instruction
def initialize_static(
    payer: Signer,
    static_acc: Empty[StaticShell],
    mint: TokenMint,
    token: Empty[TokenAccount],
    noise: u64,
    signal: u64,
    metadata: str,
):
    # oops I forgot to init static_acc
    token_account = token.init(
        payer=payer,
        seeds=['token-account'],
        mint=mint,
        authority=static_acc
    )

    # currently these would error too
    # static_acc.noise = noise
    # static_acc.signal = signal
    # static_acc.metadata = metadata
    # static_acc.bump = static_acc.bump

And you'd have authority pointing at an empty and never initialized address. I'm not sure if Anchor would catch this at runtime for you, but it won't at compile time, whereas Seahorse will as it's currently working. In a similar way you'll get an error if you try to do static_acc.noise = noise without having initialized the account, and again I think this makes sense.

@ameliatastic Would be curious if you have any thoughts on this!

[harsh4786]

Okay but then what about the .bump() method, static_acc.bump = static_acc.bump() throws an error,
or have you guys removed the bump() method? cause just passing static_acc.bump = static_acc.bump works, but i wonder if it does actually get the bump?

[mcintyre94]

Hmm nope you're right, that should be static_acc.bump(). It looks like this is a bug: you should be able to get the bump() after initializing the account, but you currently can't if you use the same variable name as with the empty one. That's not easy to fix, because when you have an Empty that includes the bump. But when you do static_acc = static_acc.init(payer = payer, seeds = ['static_Acc', metadata]) the returned account only has access to the account, not also the bump.

So basically, there's definitely a compiler issue here. Summarising:

• If you use a different name when you init the static_acc, then that name can't be used as the authority of token_account. This makes sense because of how Anchor works. We could allow you to use the static_acc as the token authority, but we'd lose a valuable compiler check (we wouldn't know the account is initialized)

• But if you use the same name, to get around that, then static_acc is now an account, not an Empty. An account doesn't have access to the bump, so you lose access to it.

In terms of possible fixes:

• We could remove our limitation that Empty[T] can't be treated as an account, for eg authority. An Empty[T] that is never initialised ends up as #[account(mut)] in Anchor. Anchor allows setting such an account to eg. authority at compile time. So we'd be matching Anchor and depending if it errors at runtime we'd do the same. But we lose the current compile time check that we have and Anchor doesn't.

• We could find a way to pass the bump so it's available.

  • Looking at the current compiler output, one option might be to pass ctx.bumps to the generated initialize_static_handler(...) function as an additional param. We could then convert attempts to get a bump to indexing this map the way we usually would in Anchor. This would allow us to get eg. ctx.bumps.get('static_acc') even when we no longer have the Empty[StaticShell] object available.
  • Another option would be to clone the static_acc: Empty[...] at the start of initialize_static_handler(...) before any compiled code. That way when you do static_acc = static_acc.init(...) we'd still have the Empty[...] with the bump available as a different variable we can read the bump from.

[mcintyre94]

Okay I'm pretty sure this works as a workaround: https://beta.solpg.io/635e9d2077ea7f12846aef0a

You can set static_acc.bump() to a variable before running static_acc.init(...) and then use that later. @harsh4786 Can you try that playground and check it's doing what you expect?

[harsh4786]

Well this does work indeed, but can you get the bump before the init() method? like does the pda get generated before you do init()?

[mcintyre94]

Yep you can. Unfortunately this is again just down to knowledge about how Anchor works, but basically the initialization actually happens before any of your other code, when you do x.init in the instruction. That's because the #[derive(Accounts)] that I mentioned in my first comment is where the init actually happens, and those accounts are all handled before the instruction code.

This is also why static_account doesn't work in your original, because that's not defined (by the instruction) until after all the accounts are initialized (before the instruction).

[harsh4786]

hmm that's interesting, well i didn't know seahorse processed the init() function first because everything in python is like sequential

[harsh4786]

Also I was facing a similar issue with token account bumps but i applied the same thing as you did with other accounts and it kinda works

[harsh4786]

Whew that was hella confusing but hopefully this discussion would be of help to others who face this issue, thanks!

[mcintyre94]

Yep this is one of the areas where the Anchor control flow leaks through a bit. Mainly that you can't define a new variable then use it in an init call.

Unless @ameliatastic disagrees I think we should leave Seahorse as-is here.

Summarising everything:

• We can't let you create static_account and then use it in authority
• We could let you create static_account and then use static_acc in authority, but we'd lose some compile time checks that IMO are useful
• As-is you have to redefine static_acc and use that in authority. You lose access to static_acc.bump() after doing this. But you can save static_acc.bump() to a variable in your code before redefining static_acc and it works correctly. I think this is reasonable and not worth a quite significant compiler change to make the bump available after.

[ameliatastic]

Wow! This issue goes surprisingly deep, and it's definitely one of the more difficult things to resolve about Seahorse. So I'm glad that you two did the detective work here.

I think in v1, I had tried to add a limitation that made it so that all .inits() had to happen at the start of an instruction. I don't even remember if it worked 100% of the time, but I dropped it from v2 anyway because it didn't feel like it really belonged. However, I knew it would allow some really subtle problems to leak through, like you guys found here. I can think of a couple of changes that would have pretty low impact on compatibility:

• Allow Empty accounts to be used as authorities, but include a check that will throw an error if an Empty account was not initialized. @mcintyre94 do you think this covers your concerns from earlier?
• Stop using Anchor account init syntax entirely. Seahorse presents account initialization as something that happens sequentially, like @harsh4786 noted, so we could just use regular CreateAccount (I think?) instructions, like Anchor has to do under the hood. High effort, high reward option I think.

Also, it's funny to see that simply redeclaring the same name static_acc = static_acc.init(...) allows everything to compile and do exactly what you want it to. That's totally unintended, but weirdly elegant somehow? It's like having multiple problems collide together and cancel each other out.

[mcintyre94]

Yep I remember that limitation, it might have stopped the trick to store the bump at the start too. I definitely agree that it didn't really belong and is nicer to have more flexibility - at least on the Seahorse side.

Allow Empty accounts to be used as authorities, but include a check that will throw an error if an Empty account was not initialized.

I think this is a nice solution, and covers my concern about forgetting to initialize.

Stop using Anchor account init syntax entirely.

I'm not at all familiar with native Solana TBH! One thing I'd be concerned about is whether we'd lose the Anchor IDL if we did this? Probably also a lot of security things that become concerns of the Seahorse compiler and currently aren't because we use Anchor's init syntax. I do agree that this would best match the way Seahorse is presenting the instruction, but at the same time I'm not sure anyone has really been limited by the way Anchor requires it to happen at the beginning.

Also, it's funny to see that simply redeclaring the same name static_acc = static_acc.init(...) allows everything to compile and do exactly what you want it to.

Agree! Took a bit of decoding to figure out that'd work lol. It feels quite weird that Rust will happily change the type of a variable like that, a lot of grumpy static languages don't and would be way more annoying to compile to!

[ameliatastic]

One thing I'd be concerned about is whether we'd lose the Anchor IDL

Would need to look into it. I think it wouldn't change things too badly, but you're right that there's probably nobody being limited by our sticking to Anchor inits.

On the topic of mixing inits and regular code, I just realized that it might lead to some security problems:

class Safe(Account):
  important_data: u64

@instruction
def do_a_little_trolling(
  signer: Signer,
  alternate: Signer,
  safe: Empty[Safe],
  use_alternate: bool
):
  # This does nothing
  if use_alternate:
    signer = alternate

  safe.init(
    payer = signer,
    seeds = ['safe', signer]
  )

Here you have some code that looks like two signers agree on whose pubkey is used as the signer seed for the Safe (via the use_alternate param, but really it does nothing. Sort of contrived, but there's definitely a problem.

Forcing inits to be at the start of an instruction solves this instance, but there's still some room for trickery.

[mcintyre94]

Hmm good point! Definitely agreed that's an issue. I can imagine less malicious examples that are just confusing too. Like conditionally setting seeds and things like that.

Maybe the answer is inits first + allow empty as authority (preferably with the check it's initialized first) then? So the original code in this issue would work, and you'd need to use a new name so you can still do static_acc.bump() after the init calls. But if you don't need the bump (or anything else from Empty) then you can re-assign to the same name.