Fix print-only flag behavior #6
Conversation
There was a problem hiding this comment.
We can do so but until now the idea was that one can use print_only flag and simulate flag together.
When the print_only is used then there should be no execution.
But it can be beneficial to get the printed version of the transaction and to find out if the transaction is ok to be executed in future. I mean the simulation could be processed and verified immediatelly on CLI even when print-only tx data is generated. WDYT?
Alright, that makes sense. In my opinion, there are two clean ways to achieve this, happy to hear your thoughts on them.
|
@martinkrecek |
There was a problem hiding this comment.
Sorry being here so unclear. But as we talk and checking the code I'm realizing the point.
It's partly on definition that I have some idea how that was meant but it's not documented and I was not able to provide that detail through the conversation.
Would you have some comment on what's your perspective on usage, see comment above.
| @@ -31,7 +31,6 @@ impl Signer for DynSigner { | |||
| } | |||
|
|
|||
| /// Keypair or Pubkey depending, could be one of that based on parameters of the CLI command. | |||
| /// For --print-only we want to permit to pass only pubkey not the keypair in real. | |||
There was a problem hiding this comment.
Don't you want to leave here some info that this is useful for print when only pubkey could be relevant?
| print_base64(&transaction_builder.instructions())?; | ||
| return Ok(()); |
There was a problem hiding this comment.
My considering is following:
When print then most probably instead of keypair will be a pubkey. That means execution cannot succeed and there is no much reason to process it - i.e., it's the behaviour at execute_anchor_builders_with_config.
When print then it could be reasonable to try to simulate to validate if the printed transaction has some chance to be executed on chain (sometimes later).
When there is no print then either execution or simulation is executed (and signature verification makes sense).
I'm still unsure if this is correct.
When print is true and simulate is true it's simulated and printed.
When print is true and simulate is false it's executed.
There was a problem hiding this comment.
I understand that with print set and simulate not set it might not be the most predictable behavior, but that's the downside of allowing the user to choose any combination and rely on them to do it reasonably. To make it a bit more predictable, print-only was renamed to print so it wouldn't make a false impression, I was even considering something like also-print/print-tx-log or something similar to make it even more explicit that there's no -only.
If we wanted to play it very safe (and you're worried about making a mistake when using the CLI) we could've gone for the other option where setting either of the flags (or the one of them which won't be removed) results in not executing anything.
Do you think I should rework it to some safer option or are we fine with the fact that the flags are simply independent of each other and you can freely print or not as you choose, regardless of whether you simulate or execute? Or do you maybe see some (reasonably complex) solution which wasn't mentioned yet?
There was a problem hiding this comment.
I think we spent pretty long time on this and so just let's put it out of the table :-)
My consideration is usage.
The most usually when one wants to print he needs to put the output to spl gov or other multisig system.
Default behaviour is now (with this PR) that --print means + execution. That most probably fails (as providing most probably the pubkey and not keypair). With printing the execution makes no sense to me.
I feel better default behaviour should be either only print or add simulate to printing as an additional step.
Anyway whatever. In case I have issues with the behaviour when I will be using to create spl gov proposal I will change the code later.
No description provided.