SIMD-0312: add CreateAccountAllowPrefund system instruction #352
Conversation
rustopian
changed the title
rustopian
changed the title
system-interface/src/instruction.rs
Outdated
| from_address: &Option<Address>, | ||
| to_address: &Address, |
There was a problem hiding this comment.
I tripped up at first with the from/to language in this case. What do you think of new_account_address & funding_account_address?
There was a problem hiding this comment.
If lamports > 0 requires -> Some(address), we could prevent an invalid argument in the typing itself with an enum:
pub enum Funding {
None,
Transfer {
from: Address,
lamports: u64,
},
}
pub fn create_account_allow_prefund(
new_account_address: &Address,
funding: Funding,
space: u64,
owner: &Address,
) -> Instruction {There was a problem hiding this comment.
Also, feels like the optional address should be second to match the instruction ordering.
There was a problem hiding this comment.
I like this new terminology. However, it does break from what is currently used by create_account.
As these are just parameter names, and I agree can be confusing, we can consider updating the names in the other functions in a subsequent PR (even the example functions in the doc comments use payer and new_account instead).
system-interface/src/instruction.rs
Outdated
| /// Create a new account **without enforcing the invariant that the account's | ||
| /// current lamports must be 0. | ||
| /// | ||
| /// This constructor is identical to [`create_account`] with the exception that it | ||
| /// **does not** check that the destination account (`to_pubkey`) has a zero | ||
| /// lamport balance prior to creation. This enables patterns where you first transfer | ||
| /// lamports to prefund an account, then use `create_account_allow_prefund` as a single | ||
| /// CPI to transfer additional lamports, allocate space, and assign ownership. | ||
| /// | ||
| /// Use [`create_account`] for typical account creation. | ||
| /// Use [`create_account_allow_prefund`] when the target account has already been | ||
| /// prefunded and you want to complete the creation process with a single CPI. | ||
| /// | ||
| /// **Safety considerations** | ||
| /// As with `allocate` and `assign` when invoked manually, this instruction can brick | ||
| /// a wallet if used incorrectly; do not pass in a wallet system account as the new | ||
| /// account. This instruction does not prevent the new account from having more | ||
| /// lamports than required for rent exemption, and all lamports will become locked. | ||
| /// | ||
| /// # Account references | ||
| /// If `lamports > 0` (meaning lamports are being transferred): | ||
| /// 0. `[WRITE, SIGNER]` New account | ||
| /// 1. `[WRITE, SIGNER]` Funding account | ||
| /// | ||
| /// If `lamports == 0` (no lamports to be transferred), you may omit funding account: | ||
| /// 0. `[WRITE, SIGNER]` New account |
|
@grod220 I added your suggested This file needs a bit more unrelated touch up (to use solana-sdk-ids for example), but I'll handle that in a separate chores PR. |
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Looks great! I'm not sure about the extra enum, but maybe I can be convinced
system-interface/src/instruction.rs
Outdated
| #[derive(Clone, Debug, Eq, PartialEq)] | ||
| pub enum Funding { | ||
| None, | ||
| Transfer { from: Address, lamports: u64 }, |
There was a problem hiding this comment.
To be honest, I'm not sure if this is a great idea. It's currently passed in by value, which forces a copy of the pubkey and lamports.
Even if it's passed by reference, users are forced to create an intermediate struct just to create the instruction.
Finally, the resulting code is more verbose than just taking in lamports and Option<&Address> always.
Is that worth the tradeoff to make it impossible for someone to specify an amount of lamports without also specifying a funder? Maybe! But I'm not convinced.
There was a problem hiding this comment.
Cc @grod220 - how strongly do you feel on the Funding type protection here?
I love such things, but since we were already checking the Option<Address> in the original anyway...
if lamports > 0 {
if let Some(from) = payer {
account_metas.push(
AccountMeta::new(*from, true)
);
} // could `else {error}` here
}
...maybe a middle ground is to just error here? Better UX than failing downstream, no extra type.
At the cost of returning a Result-wrapped Instruction tho.
There was a problem hiding this comment.
I don't feel strongly, especially if it breaks expected patterns of the crate. I suppose it's more of a question where we want this case to throw:
- At compile time -> use enum
- At runtime call site -> add error to this helper
- At program execution -> no guards
There was a problem hiding this comment.
Let's see if Option<(Address, u64>) is good. Then if is_none(), no transfer, no funding account. No extra type, just a tuple, but still keeps the u64 and Address linked.
rustopian
changed the title
3 participants
SIMD
Adds the
CreateAccountAllowPrefundinstruction. This may be a breaking change in rare cases, since any exhaustive pattern matching will now have an additional variant ofSystemInstructionto deal with.