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

[AH] Ensure either the seller or the authority is passed as signer on the sell instruction #912

Merged
merged 3 commits into from
Dec 22, 2022

Conversation

lorisleiva
Copy link
Contributor

@lorisleiva lorisleiva commented Dec 12, 2022

  • Creates a new SaleRequiresExactlyOneSigner error.
  • Throw the new error when both the seller and the authority are passed as signers in the sell instruction.

See #911

@github-actions
Copy link
Contributor

Workflow Verify package library found differences when running yarn api:gen in the JS lib for auction-house. Please see the job for more details: https://github.com/metaplex-foundation/metaplex-program-library/actions/runs/3676534980.

@lorisleiva lorisleiva marked this pull request as ready for review December 12, 2022 14:17
@lorisleiva lorisleiva requested a review from a team as a code owner December 12, 2022 14:17
@austbot
Copy link
Contributor

austbot commented Dec 12, 2022

This change seems good, Is this motivated by web3 js throwing an error when a signer is not needed?

@lorisleiva
Copy link
Contributor Author

Hey @austbot,

No because the program accepts the scenario where both the seller and the authority are passed as signers.

When that's the case, it just happens that:

  • The fee_payer variable will be set to authority (See code).
  • But the seller will still approve a new delegate authority on the token (See code).

The motivation behind this PR is that the invariant defined here don't match the code. Even though this ends up not causing a security threat, it's best to be explicit about what we expect to avoid bad surprises in the future.

This was raised by issue #911.

Copy link
Contributor

@stegaBOB stegaBOB left a comment

Choose a reason for hiding this comment

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

Looks good to me. Easy approve on this one!

@samuelvanderwaal
Copy link
Contributor

@lorisleiva this looks good to merge to me

@lorisleiva
Copy link
Contributor Author

patch version

@lorisleiva lorisleiva merged commit 3b975c3 into master Dec 22, 2022
@samuelvanderwaal samuelvanderwaal deleted the loris/ah-assert-one-signer branch December 22, 2022 22:22
@jdnichollsc
Copy link

Hey folks, what do you think about this PR? <3 #1059

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants