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

Extend shank instructions to work with Instruction struct variants #32

Merged
merged 4 commits into from
Aug 21, 2022

Conversation

ngundotra
Copy link
Contributor

@ngundotra ngundotra commented Aug 19, 2022

This PR adds two features to ShankInstructions:

  1. Parses Instruction variants with named arguments
  2. Parses Instruction variants with multiple unnamed arguments (with names instructionArgs0, instructionArgs1, etc)

This is necessary to use Shank with SPL instructions. Closes #33

Example: Instruction variant with named arguments

Instructions with named arguments now works. The following

#[derive(ShankInstruction)]
pub enum Instruction {
    #[account(0, name = "creator", sig)]
    #[account(1, name = "thing", mut)]
    CreateThing {
        some_args: SomeArgs,
        other_args: OtherArgs,
    },
    #[account(0, name = "creator", sig)]
    CloseThing(Option<u8>),
}

compiles to:

{
  "version": "",
  "name": "",
  "instructions": [
    {
      "name": "CreateThing",
      "accounts": [
        {
          "name": "creator",
          "isMut": false,
          "isSigner": true
        },
        {
          "name": "thing",
          "isMut": true,
          "isSigner": false
        }
      ],
      "args": [
        {
          "name": "someArgs",
          "type": {
            "defined": "SomeArgs"
          }
        },
        {
          "name": "otherArgs",
          "type": {
            "defined": "OtherArgs"
          }
        }
      ],
      "discriminant": {
        "type": "u8",
        "value": 0
      }
    },
    {
      "name": "CloseThing",
      "accounts": [
        {
          "name": "creator",
          "isMut": false,
          "isSigner": true
        }
      ],
      "args": [
        {
          "name": "instructionArgs",
          "type": {
            "option": "u8"
          }
        }
      ],
      "discriminant": {
        "type": "u8",
        "value": 1
      }
    }
  ],
  "metadata": {
    "origin": "shank"
  }
}

Example: Instruction variant with multiple unnamed arguments

Previously this would throw with an error saying "An Instruction can only have one arg field". But now it works. The following

#[derive(ShankInstruction)]
pub enum Instruction {
    #[account(0, name = "creator", sig)]
    CloseThing(Option<u8>, ComplexArgs, ComplexArgs),
}

compiles to

{
  "version": "",
  "name": "",
  "instructions": [
    {
      "name": "CloseThing",
      "accounts": [
        {
          "name": "creator",
          "isMut": false,
          "isSigner": true
        }
      ],
      "args": [
        {
          "name": "instructionArgs0",
          "type": {
            "option": "u8"
          }
        },
        {
          "name": "instructionArgs1",
          "type": {
            "defined": "ComplexArgs"
          }
        },
        {
          "name": "instructionArgs2",
          "type": {
            "defined": "ComplexArgs"
          }
        }
      ],
      "discriminant": {
        "type": "u8",
        "value": 0
      }
    }
  ],
  "metadata": {
    "origin": "shank"
  }
}

Todo:

  • Add change log

- they are inside an instruction thus that context is already provided
- shorter names result in better generated code with solita
Copy link
Contributor

@thlorenz thlorenz left a comment

Choose a reason for hiding this comment

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

Very nice work, I reviewed and also ran the IDL against solita to see what the generated code looks like.
One improvement we could do in the future is to represent the instructions args as tuples, i.e. [u8, String, ComplexType] instead, but for now this solution is fine.

I fixed one minor nit (instructionArgs seemed a bit long to me, also when we're talking about one arg it shouldn't be named instructionArgs0, but instructionArg0 or just arg0).

Aside from that all checks out and I'm happy to merge if you're fine with my modification.

@ngundotra
Copy link
Contributor Author

Absolutely! Go for it. Thanks Thorsten!

@thlorenz thlorenz merged commit a5bd471 into metaplex-foundation:master Aug 21, 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.

[feat]: Support multiple instruction args
2 participants