Conversation
There was a problem hiding this comment.
I would argue that any of the changes for the programs/settlement/idl/client should be dropped from this PR and possibly moved to a new PR. While we do already have support for the close-to-identical function CreateOrder explicitly within the generated JS library, no integrator is going to be using the CreateWithrawalOrder and I am pretty sure the value of having this extra code is pretty low.
I would consider renaming create_withdrawal_order to create_self_order since it is not technically required that the order need be a withdrawal. Technically the order could simply be for some internal accounting purpose in the future instead, so the naming doesn't really explain fundamentally what is happening.
Arguably unlike CreateOrder, it would be useful to be able to create many orders at once with this instruction (ex. we basically know that in a single fee withdrawal cycle there could be tens of different tokens that need to be converted into SOL). This would probably be pretty simple too, as order_pda account could simply by repeated and intent bytes could be a multiplier of EncodedOrderIntent length. Inside the processor it would be a basic for loop.
| /// Allocates a per-order PDA (see [`crate::pda::order`]) for an order owned by | ||
| /// the settlement state PDA, so the fees that accumulate in the buffer accounts | ||
| /// can be sold through a regular settlement. The PDA's storage layout is the | ||
| /// same as any other order's, [`crate::data::order::EncodedOrderAccount`]. | ||
| /// | ||
| /// Unlike [`CreateOrder`](crate::instruction::create_order::CreateOrder), the | ||
| /// order isn't authenticated by its owner's signature: the owner is the | ||
| /// settlement state PDA, which has no key. Instead the instruction is gated by | ||
| /// the [`WithdrawalAuthority`](crate::Role::WithdrawalAuthority), a privileged | ||
| /// account that signs it. The program forces `intent.owner` to be the state | ||
| /// PDA, so a withdrawal order can only sell funds stored in the buffers. | ||
| /// Otherwise, the order this function created is a normal order and settles | ||
| /// through the standard `BeginSettle`/`FinalizeSettle` flow. | ||
| /// | ||
| /// The withdrawal authority is trusted to choose sound order parameters. | ||
| /// The only enforced parameters are `created_on_chain` (should be true) and | ||
| /// the owner (should be the state PDA). | ||
| /// | ||
| /// `payer` funds the new order PDA's rent and is recorded as its `created_by` | ||
| /// address, so `ReclaimOrder` refunds the rent there. | ||
| /// | ||
| /// Wire format: `[discriminator=10, ..intent bytes]`, | ||
| /// `1 + EncodedOrderIntent::SIZE` bytes. Required accounts: | ||
| /// `[authority (S), payer (W,S), state_pda (R), order_pda (W), | ||
| /// system_program (R)]`. The system program needs to be available but doesn't | ||
| /// need to sit at that specific position, unlike the others. |
There was a problem hiding this comment.
Can we simplify this giant description down to just "Same as CreateOrder, but only the withdraw_authority can call and the owner is set to state_pda. The purpose is to allow direct management of the settlement program's buffer funds." or so. Looking at the parameters (and processor) they are the same so its easiest to understand this way
There was a problem hiding this comment.
Simplified a bit: 11b7f36
But I'd still like to keep it a bit verbose for consistency with the other instructions. I agree it says a lot for a code comment, but as documentation for the package I think it's helpful to describe the instruction in detail. (In general I always wondered whether the more verbose comments should be moved to the interface instead of having them in the client.)
| pub struct CreateWithdrawalOrderInput<'a, A> { | ||
| pub intent_bytes: [u8; EncodedOrderIntent::SIZE], | ||
| pub authority: &'a A, | ||
| pub payer: &'a A, | ||
| pub state_pda: &'a A, | ||
| pub order_pda: &'a A, | ||
| } |
There was a problem hiding this comment.
the fields between CreateOrderInput and CreateWithdrawalOrderInput are so similar that you could hypothetically use it here as a nested struct and then include state_pda (the only "added" parameter). This would even allow validating off of the same facilities already provided in the create_order.rs instruction parser.
There was a problem hiding this comment.
I tried it but I don't really like the result: ddc6e91
I'd say it's nicer to keep everything flat for consistency. Also, I dislike that decoding requires you to do owner: state_pda,, meaning that the caller needs to be aware that the variable has another real name.
I don't particularly care about having
I'm fine with this, on the other hand the extra IDL code is about 80 lines so I'm not sure it's worth splitting it off. Will still do it if requested again. (The IDL itself needs to be changed if we want CI to pass, this is why we don't save that many diff lines.)
I didn't mention it but this is specifically out of scope. It would require rethinking the instruction and I don't think it's that easy. This PR is already >1k lines. |
fedgiac
changed the title
…settlement-program-dedicated-order
kaze-cow
left a comment
kaze-cow
left a comment
There was a problem hiding this comment.
very minor comments remain
2 participants
Add the to place a self order: an order owned by the settlement state PDA that sells tokens sitting in a buffer (or, strictly speaking, any token controlled by the state PDA). This is gated by the withdrawal authority introduced in #150 (which has been renamed to self-order authority after a reviewer's request).
How a self order is placed
CreateSelfOrderbasically copiesCreateOrderexcept for the initial owner check. In particular, it takes a full order intent as part of the input bytes. The main difference is that the order must be owned by the state PDA and the withdraw authority (not the owner) must sign the transaction.The owner check in the intent is extremely important: without it, the withdraw authority could steal funds from any user who delegated the settlement PDA.
Why is the PR so big?
Not that much is happening in the actual contract. I tried to isolate unrelated changes in #152, but we didn't gain much, most of the code is still new. A good part of this is the IDL-related changes. The tests are also relatively large.
How to test
CI should be enough, new tests have been added. Also for the IDL.