Program macro to generate helper fns and documentation

[CriesofCarrots]

Problem

There's a lot of information duplicated between program instruction enums and helper functions that build Instructions. Also, it's difficult to parse on-chain instructions and get useful information about the expected accounts, as that information is buried in the documentation in the aforementioned enum. Design proposal #10763 suggests addressing both issues by adding a procedural macro that would parse an annotated Instruction enum to generate needed helper functions as well as a 2nd enum that exposes account information. This would also have the benefit of enforcing consistency across program Instruction enum docs.

Summary of Changes

• Add instructions attribute procedural macro

Toward #10476

Notes:

• trybuild is a neat tool for catching compilation failures and ensuring error messages are helpful and descriptive. However, it's possible there could be issues with our monorepo due to Ensure tests are built with same dependencies as parent crate dtolnay/trybuild#53. (I ran into those bincode errors we saw on master recently due to a newer version of bincode in my .cargo/registry/cache being pulled into the trybuild compilation) If it seems flaky in CI or for devs, I'll remove
• TODO:
  • Add skip variant-level attribute

[CriesofCarrots]

@garious , @jackcmay, one thing I wanted to run by you:

I originally wrote the helper functions to use Instruction::new(), but that requires the program instruction enum to implement Serialize... spl_token, for example, does not. e104a09 updates the macro to require the enum to either derive Serialize or implement a custom serialize() method. Does this seem reasonable?

[codecov]

Codecov Report

Merging #10826 into master will decrease coverage by 0.1%.
The diff coverage is 56.3%.

@@            Coverage Diff            @@
##           master   #10826     +/-   ##
=========================================
- Coverage    81.8%    81.7%   -0.2%     
=========================================
  Files         303      304      +1     
  Lines       71067    71406    +339     
=========================================
+ Hits        58194    58389    +195     
- Misses      12873    13017    +144     

[jackcmay]

@CriesofCarrots @jstarry brought this up a while ago and contrary to my last comment in the issue I agree, the Instruction::new's built-in dependency on serializing is probably not what we want. I don't see why we should enforce any kind of structure on instruction data at this layer.
#5970

[CriesofCarrots]

I don't see why we should enforce any kind of structure on instruction data at this layer.

The custom serialize isn't enforcing any kind of structure, just that the instruction have a serialize method of some kind.
If that is undesirable, the alternative is to have these generated helper functions expect Vec<u8> -- ie. already serialized data -- instead of a program instruction. That seems less clean to me, but more flexible.

[jackcmay]

Taking a Vec<u8> or &[u8] seems cleaner to me. IMO the instruction should not assume any structure or form about data, enforcing Serialize trait makes unnecessary assumptions about the form or source of the data. To the instruction, the data should be a purely transparent chunk of bytes. The creator of the instruction should have a client/server type layer on top of instruction that concretely knows the structure and form of data.

[garious]

Changing it to Vec<u8> is fine. The Serialize was meant to be a convenience feature, not a constraint.

[CriesofCarrots]

@garious , this is updated. Please let me know if there's something else you'd like to see before review.

[garious]

Looking back at the proposal, I see this syntax made it in, but I still find it really confusing. Remind me why we can't write (from_account, SIGNER | WRITABLE, desc = "Funding account")?

[CriesofCarrots]

This is using the standard attribute syntax for a list (of accounts/names), which each carry a list of account attributes.
In addition to the "standardization" aspect, it's significantly easier to parse the attributes in the macro if they can be mapped into syn library Meta types.

But if you feel really strongly about this, I'll write a custom parser.

One thing that I'm not crazy about with your proposed syntax is the strict ordering that's required inside the parens in order to identify the account name. As it is, the account name is unambiguous, because it is the item Path, and account attributes inside the parens can be listed in any order and remain parsable.

[garious]

I'm looking for as little custom parsing as possible and that wherever possible, pass in Rust expressions. I don't want to learn a new DSL to annotate this enum.

[garious]

Maybe this is possible?

use solana_sdk_program_macros::{SIGNER, WRITABLE};
...
#[accounts(
       (from_account, SIGNER | WRITABLE, desc: "Funding account")
  )]

So a tuple of an identifier, an expression that evaluates to a u64, and then key-value pairs that use the struct field syntax.

[CriesofCarrots]

Sure, it's possible. I think this is approx the same amount of extra handling as your previous version.

I'm not sure whether it's easy to actually evaluate the SIGNER | WRITABLE expression in the macro. I'll have to look into that.

[garious]

Why no use solana_sdk_program_macros::{SIGNER, WRITABLE}; in here?

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This stale pull request has been automatically closed. Thank you for your contributions.