add CreateAccountAllowPrefund builder#307
Conversation
| ) -> Result<Self, ProgramError> { | ||
| let rent = Rent::from_account_view(rent_sysvar)?; | ||
| let required_lamports = rent.try_minimum_balance(space as usize)?; | ||
| let lamports = required_lamports.saturating_sub(to.lamports()); |
There was a problem hiding this comment.
This does allow a wallet to be bricked with a large lamport balance, which is explicitly the risk developers must avoid when using CreateAccountAllowPrefund.
But measures such as checked_sub here seem inappropriate, since the lamports pre-funded may be a little bit more than required rent, and any defensive check for an arbitrary amount of overage in lamports adds compute.
There was a problem hiding this comment.
Could we document this "risk"?
rustopian
force-pushed
the
create-account-allow-prefund
branch
2 times, most recently
from
6eba538 to
a8fc962
Compare
rustopian
force-pushed
the
create-account-allow-prefund
branch
from
a8fc962 to
9db0e2d
Compare
rustopian
changed the title
| /// Funding account and lamports to transfer to the new account. | ||
| pub payer_and_lamports: Option<(&'a AccountView, u64)>, |
There was a problem hiding this comment.
Wondering whether would be better to keep the same "interface" as CreateAccount or not – i.e., from and lamports as separate fields.
There was a problem hiding this comment.
I thought about this, but it mirrors the same change elsewhere, and semantically keeps the two together
Up to you tho; will change if you prefer
There was a problem hiding this comment.
Make sense. What do you think if we are more explicit about the role of these values?
/// Funding lamports to transfer into a newly created account.
pub struct Funding<'a> {
/// Funding account.
pub from: &'a AccountView,
/// Number of lamports to transfer to the new account.
pub lamports: u64,
}Then the field would be:
pub funding: Option<Funding<'a>>,And you would use as:
CreateAccountPrefund {
funding: Funding {
from,
lamports: 1_000_000_000,
},
...
}
| AccountView, Address, ProgramResult, | ||
| }; | ||
|
|
||
| /// Create a new account without the `lamports==0` assertion. |
There was a problem hiding this comment.
nit: Can we expand a little bit the description?
| /// Create a new account without the `lamports==0` assertion. | |
| /// Create a new account, which can be prefunded. | |
| /// | |
| /// While [`super::CreateAccount`] can only be used when the account being created has | |
| /// no lamports, `CreatePrefund` relaxes this requirement. |
| /// This instruction does not warn if the account has more than enough lamports; large | ||
| /// lamport balances can be frozen by `CreateAccountAllowPrefund` if used incorrectly. |
There was a problem hiding this comment.
nit: Maybe we should put this under a "# Important" section and expand a bit on the risk of "bricking" a wallet account if it is mistakenly passed as the account to create.
febo
left a comment
febo
left a comment
There was a problem hiding this comment.
Looks good, left a couple of suggestions to discuss.
2 participants
CreateAccountAllowPrefundis now available insolana-system-interfaceandagave. This adds a builder in pinocchio analogous to the existing system instruction builders.