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

solana: Handle transferring mint authority using SPL Multisig #587

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

nvsriram
Copy link
Collaborator

@nvsriram nvsriram commented Feb 3, 2025

Context:

Currently, there are instructions to safely handle transferring token mint authority to-and-from the NTT Manager - token_authority PDA.

However, with SPL Multisig support, these instructions need to be modified to handle all cases.

List of Abbreviations:

  • Multisig(...) = any generic SPL Multisig with signers …
  • Multisig(TA, ...) = SPL Multisig with m = 1 and token_authority PDA as one of its required signers
  • TA = token_authority PDA
  • TA/Multisig(TA, ...) = Representation of NTT Manager
    • Could be token_authority PDA or SPL Multisig with m = 1 and token_authority PDA as one of its required signers

New Proposed Instructions:

Transfer Mint Authority OUT:

  • set_token_authority_one_step_unchecked: TA/Multisig(TA, ...) -> any non-signer
  • set_token_authority: TA/Multisig(TA, ...) -> non-signer S
    • claim_token_authority: claimed by S as signer
    • claim_token_authority_to_multisig: claimed by S = Multisig(...)
      • ctx.remaining_accounts act as required signers of Multisig(...)
    • revert_token_authority: reverted back to TA/Multisig(TA, ...)

Transfer Mint Authority IN:

  • accept_token_authority: signer S -> TA/Multisig(TA, ...)
  • accept_token_authority_from_multisig: Multisig(...) -> TA/Multisig(TA, ...)
    • ctx.remaining_accounts act as required signers of Multisig(...)

Implementation Details:

  • When the NTT Manager is Multisig(TA, ...): Option<InterfaceAccount<'info, SplMultisig>> is used to avoid additional explicit instructions
  • When Multisig(...) needs to sign/validate: ctx.remaining_accounts is treated as required signers

tl;dr:

This PR adds:

  • 7 mint authority update instructions to handle all SPL Multisig cases (described above)
  • Add NTT namespace TS helpers for the instructions
  • Update TS test to cover multiple mint authority transfer instances
  • Update IDL

@nik-suri nik-suri requested a review from nonergodic February 4, 2025 17:26
nonergodic
nonergodic previously approved these changes Feb 5, 2025
Copy link
Collaborator

@nonergodic nonergodic left a comment

Choose a reason for hiding this comment

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

Only nits.

One other thing that I noticed that isn't part of the changes here:
The address constraint on the owner signer in RevertTokenAuthority has no custom error, while the other address checks do. I assume this is because usually these type of constraints are checked via has_one which isn't possible here - but I figured I'd flag it here regardless (might be worth a short comment).

Copy link
Collaborator

Choose a reason for hiding this comment

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

The code in here could really profit from a bunch of DRY-ness. See e.g. TBRv3 tests that use various helpers under the hood.

config: NttBindings.Config<IdlVersion>,
args: {
currentMultisigAuthority: PublicKey;
additionalSigners: PublicKey[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
additionalSigners: PublicKey[];
additionalSigners: readonly PublicKey[];

args: {
rentPayer: PublicKey;
newMultisigAuthority: PublicKey;
additionalSigners: PublicKey[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
additionalSigners: PublicKey[];
additionalSigners: readonly PublicKey[];

@nvsriram
Copy link
Collaborator Author

nvsriram commented Feb 6, 2025

Context:

I refactored out the common functions into claim_from_token_authority and claim_from_multisig_token_authority to avoid repetition. However, I notice admin.rs getting quite lengthy so I would like to refactor it into its own module and then each individual ix (or set of related ix's) can be its own file (or submodule).

Example Refactor:

  • set_peer, register_transceiver, set_outbound_limit, and all the other one-off ix's could be their own file.
  • transfer_ownership_one_step_unchecked, transfer_ownership, and claim_ownership could be one file.
  • accept_token_authority and accept_token_authority_from_multisig could be one file.
  • set_token_authority_one_step_unchecked, set_token_authority, claim_token_authority, claim_token_authority_to_multisig, and revert_token_authority could be one file.

Alternative Refactor:

  • register_transceiver, set_outbound_limit, and all the other one-off ix's could be their own file.
  • transfer_ownership could be its own submodule to separate the "transfer" and "claim" ix's into individual files.
  • handle_token_authority could be its own submodule to separate the "accept", "set", and "claim"/"revert" ix's into individual files.

tl;dr:

  1. Is there a reason not to refactor admin.rs ix's and instead leave it as one lengthy file?
  2. If a refactor is recommended, which option is preferred:
    1. Every ix gets an individual file
    2. Every ix gets an individual file and we use submodules to group related ix's
    3. Every set of related ix's gets an individual file (see example refactor above)
    4. Every set of related ix's gets an individual file but if the set is too large, it gets a submodule (see alternative refactor above)
    5. Variation on one of the above/ None of the above

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.

2 participants