Repository navigation
Conversation
The funder pays the 5,000 lamport base fee for every required signature, and the count comes from the client-controlled message header. Placement gave it a minimum of two (funder and owner) and no maximum, so a client could pad the header with throwaway signed keypairs and inflate the fee. A creation whose header does not require exactly two signers is now rejected with InvalidTransaction.
|
Claude finished @tilacog's task in 22s —— View job Review
I found nothing that needs a change. The check that the header names exactly two required signers runs right after the funder is confirmed as The tests look right:
|
|
🤖 Claude reviewed this PR. Any issues are posted as inline comments on the relevant lines.
Won't run when: the PR is a draft, the author is a bot or lacks write access, or the |
| } | ||
| if message.header().num_required_signatures != 2 { | ||
| return Err(PlacementError::InvalidTransaction( | ||
| "a creation transaction must have exactly two required signers", |
There was a problem hiding this comment.
add a bit more info
| "a creation transaction must have exactly two required signers", | |
| "a creation transaction must have exactly two required signers: the funder and the owner", |
or similar
There was a problem hiding this comment.
Agree, plus maybe a comment/reminder on the reason for that.
| assert!(matches!( | ||
| validate(&sponsoring, &transaction, ValidationParameters::default()), | ||
| Err(PlacementError::InvalidTransaction( | ||
| "a creation transaction must have exactly two required signers" | ||
| )) | ||
| )); |
There was a problem hiding this comment.
this should now be possible
| assert!(matches!( | |
| validate(&sponsoring, &transaction, ValidationParameters::default()), | |
| Err(PlacementError::InvalidTransaction( | |
| "a creation transaction must have exactly two required signers" | |
| )) | |
| )); | |
| assert_matches!( | |
| validate(&sponsoring, &transaction, ValidationParameters::default()), | |
| Err(PlacementError::InvalidTransaction( | |
| .. | |
| )) | |
| ); |
There was a problem hiding this comment.
With .. this test probably passes without the new rule too. The message has no instructions, so validate already answers InvalidTransaction("the transaction carries no instructions"). assert_matches! is fine, but, IMO, the string should stay.
| if keys.first() != Some(&sponsoring.funder) { | ||
| return Err(PlacementError::WrongFeePayer); | ||
| } | ||
| if message.header().num_required_signatures != 2 { |
There was a problem hiding this comment.
A creation with a two-signer header and three signature slots still passes here, right?
| assert!(matches!( | ||
| validate(&sponsoring, &transaction, ValidationParameters::default()), | ||
| Err(PlacementError::InvalidTransaction( | ||
| "a creation transaction must have exactly two required signers" | ||
| )) | ||
| )); |
There was a problem hiding this comment.
With .. this test probably passes without the new rule too. The message has no instructions, so validate already answers InvalidTransaction("the transaction carries no instructions"). assert_matches! is fine, but, IMO, the string should stay.
| } | ||
| if message.header().num_required_signatures != 2 { | ||
| return Err(PlacementError::InvalidTransaction( | ||
| "a creation transaction must have exactly two required signers", |
There was a problem hiding this comment.
Agree, plus maybe a comment/reminder on the reason for that.
Description
validatereadnum_required_signaturesfrom the client-controlled message header and used it only as a loop bound. A client could pad the header with throwaway keypairs that each signed, and the funder paid 5,000 lamports of base fee per extra signature. A sponsored creation now must name exactly two required signers, the funder and the owner.Changes
create_order/mod.rs:validatereturnsInvalidTransaction("a creation transaction must have exactly two required signers") when the header does not name exactly two required signers.tests/api.rs:create_order_requires_the_owner_as_signerused a one-signer header, which the new rule refuses first. It now builds a two-signer creation with a stranger in the owner's slot and still expectsInvalidSignature.How to test
cargo nextest run -p solana-orderbooka_header_without_exactly_two_required_signers_is_rejectedrefuses headers of 1 and 3.create_order_rejects_an_extra_required_signerrefuses a third signer with a valid signature.Related Issues
BE-525