anchor icon indicating copy to clipboard operation
anchor copied to clipboard

lang: Implement Immutable<> account type that gives compilation error on mutation unlike Account<>

Open snawaz opened this issue 1 year ago • 7 comments

Currently, if we have this supposedly immutable account:

pub account: Account<'info, Proposal>, // note: it does not have `#[account(mut)]`

then we can still modify its value:

ctx.accounts.account.title = "but this change does not persist".into();

and we do not get compilation error, not even runtime error! The change made to the account does not persist though. It's a high point of frustration for many developers (including me!). I implemented Immutable<'info, T> which solves this problem and we get compile-time error.

Here is a small demo and as you can see the editor itself marks the errors, at line 21 and 23 (of course with the help of a working language-server):

image

So, now we do not only get compilation errors, we also get two Help messages to improve the above code:

  • One help to use Immutable<'info, Proposal> for account field which is supposed to be immutable. It's quite helpful message.
  • And the other isn't that interesting, as it simply suggests us to remove [account(mut)] to avoid having a paradox.
image

Please let me know if I miss anything! If this looks good, I'll edit the docs in immutable.rs (which is a copy of account.rs; in fact, I copied this file, renaming the type and removed impl DerefMut).

snawaz avatar Jun 03 '23 20:06 snawaz

@snawaz is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar Jun 03 '23 20:06 vercel[bot]

This PR attempts to resolve this issue: https://github.com/coral-xyz/anchor/issues/326

snawaz avatar Jun 03 '23 20:06 snawaz

Currently, if we have this supposedly immutable account:

pub account: Account<'info, Proposal>, // note: it does not have `#[account(mut)]`

then we can still modify its value:

ctx.accounts.account.title = "but this change does not persist".into();

and we do not get compilation error, not even runtime error! The change made to the account does not persist though. It's a high point of frustration for many developers (including me!). I implemented Immutable<'info, T> which solves this problem and we get compile-time error.

I agree this is a weird behavior but adding a brand new account type that needs to be kept in sync with Account for a strictly compile-time issue is a bit too much.

What's the blocker for making Account behave like your Immutable type and making #[account(mut)] change the account's mutability in compile-time as well?

acheroncrypto avatar Jun 05 '23 10:06 acheroncrypto

What's the blocker for making Account behave like your Immutable type and making #[account(mut)] change the account's mutability in compile-time as well?

The problem is: Account<T> implements DerefMut which makes it mutable irrespective of #[account(mut)]. So the only way to make it immutable is to NOT implement DerefMut for it. If we conditionally generate the implementation of DerefMut based on the presence of #[account(mut)] on a field, even then it wont work, for the following two reasons:

  • #[account(mut)] is in the user code. That means, the impl DerefMut for Account<T> will be generated in the use code. But this wont compile, because neither DerefMut nor Account type is defined by the user code. Rust requires at least one of them to be defined in the crate in order to compile.
  • Second, even if the generated impl DerefMut for Account<> compiles, it'll make all instances of Account<T> mutable. After all, DerefMut is a implemented for a type, not for an instance or a field.

However this analysis leads me to another potential solution.

We can try this solution:

  • anchor side changes:

    struct Mutable<T>(T); // where T is a type that implements `#[account]`, so we can use some constraint here
    
    impl<T> AsMut<T> for Mutable<T> { .. } 
    
    impl<U, T: AsMut<U>> DerefMut for Account<'a, T> { }
    

    That makes Account<T> immutable by default, but Account<Mutable<T>> is mutable.

  • user side changes:

    #[account(mut)]
    pub account: Account<'info, Proposal>
    

    should translate to:

    pub account: Account<'info, Mutable<Proposal>>  // account(mut) can make this transformation
    

But then I guess, now Mutable needs to implement (some or all) of the traits that #[account] implements, so that it can be passed to Account. Also, in the error message, the user will not see Account<'info, Proposal>, instead they will see Account<'info, Mutable<Proposal>>

Is this a better solution (assuming it'll work)?

snawaz avatar Jun 05 '23 13:06 snawaz

I agree this is a weird behavior but adding a brand new account type that needs to be kept in sync with Account for a strictly compile-time issue is a bit too much.

The other arguments could be.. we have many other types which ensure certain things, for example, Signer and Program. As for the syncing issue, I guess, that can be solved, if we use macro_rules which generates the common stuffs for both types.

snawaz avatar Jun 05 '23 13:06 snawaz

The other arguments could be.. we have many other types which ensure certain things, for example, Signer and Program. As for the syncing issue, I guess, that can be solved, if we use macro_rules which generates the common stuffs for both types.

Yes, you are right on that but there is a subtle difference. Let's take Signer as an example, an account is not a signer by default and using Signer marks the account as signer but for Immutable, an account is not mutable by default(though not compile-time) and using Immutable marks the account immutable. The difference is that the account is already immutable by default and explicitly marking an account Immutable feels a bit off considering Solana program runtime and Rust both have immutability by default.

We can try this solution:

  • anchor side changes:

    struct Mutable<T>(T); // where T is a type that implements `#[account]`, so we can use some constraint here
    
    impl<T> AsMut<T> for Mutable<T> { .. } 
    
    impl<U, T: AsMut<U>> DerefMut for Account<'a, T> { }
    

    That makes Account<T> immutable by default, but Account<Mutable<T>> is mutable.

  • user side changes:

    #[account(mut)]
    pub account: Account<'info, Proposal>
    

    should translate to:

    pub account: Account<'info, Mutable<Proposal>>  // account(mut) can make this transformation
    

But then I guess, now Mutable needs to implement (some or all) of the traits that #[account] implements, so that it can be passed to Account. Also, in the error message, the user will not see Account<'info, Proposal>, instead they will see Account<'info, Mutable<Proposal>>

Is this a better solution (assuming it'll work)?

I think this looks like a much better solution as we keep the immutability by default without needing to make it explicit.

acheroncrypto avatar Jun 06 '23 07:06 acheroncrypto

I think this looks like a much better solution as we keep the immutability by default without needing to make it explicit.

I tried to implement this and realized that this solution cannot be implemented, because derive macros cannot change the fields of the structs (what they can do is, generate additional code without modifying the structs themselves), and this solution requires translating user code, from #[account(mut)] Account<'info, T> to Account<'info, Mutable<T>>, to enable mutation — (and by default Account<'info, T> would be immutable).

Even if it were possible to the change the fields, the solution is pointlessly complex, and overall not neat compared to the alternative, because Mutable<T> has to be like an account, and thus IDL needs to be supported for a generic type as well, which is currently not supported.

Yes, you are right on that but there is a subtle difference. Let's take Signer as an example, an account is not a signer by default and using Signer marks the account as signer but for Immutable, an account is not mutable by default(though not compile-time) and using Immutable marks the account immutable. The difference is that the account is already immutable by default and explicitly marking an account Immutable feels a bit off considering Solana program runtime and Rust both have immutability by default.

I understand your point of view. But this can be fixed by mere renaming:

  • Keep Account<> as it is. Only remove DerefMut implementation for it. That'd make it immutable. That is what it should be.
  • Then we can rename the type, Immutable<>, introduced this PR, to Mutable<> and implement DerefMut for it.

Since Mutable<> = Account<> + DerefMut, we can add a simple macro_rules that can implement the common stuff for both types, and that'd solve the "keep-both-types-in-sync" problem.

snawaz avatar Jun 15 '23 17:06 snawaz