test: meta transaction account creation scenarios #8568
Conversation
Integration tests for different ways of creating accounts. This adds some test utilities for implicit accounts, and it fixes the meta tx checker functions to also work with implicit accounts, as they don't use the account name as seed for the access key.
| pub fn from_implicit_account(account_id: &AccountId) -> Self { | ||
| assert!(account_id.is_implicit()); | ||
| let mut public_key_data = Vec::with_capacity(33); | ||
| public_key_data.push(KeyType::ED25519 as u8); |
There was a problem hiding this comment.
Parts of this code seem to be taken/copied almost verbatim from runtime/runtime/src/actions.rs so this would perhaps be better off living in a common (non-test-utils) code that either tests or code could use.
(If that's not feasible for any reason, the actions.rs code could be adjusted at least to replace the magic 0 constant with the KeyType as is done here, as you're touching the relevant code anyway…)
There was a problem hiding this comment.
Yeah, we even have another instance of this code copied into near-mirror.
But I don't like editing production code in test-only PRs that are by themself already quite complicated. I'd rather fix this in a separate issue:
test-utils/testlib/src/fees_utils.rs
Outdated
| self.cfg.fee(ActionCosts::transfer).send_fee(sir) | ||
| + self.cfg.fee(ActionCosts::create_account).send_fee(sir) | ||
| + self.cfg.fee(ActionCosts::add_full_access_key).send_fee(sir) |
There was a problem hiding this comment.
Huh, do I understand correctly this is effectively making any transfer to an implicit account via a meta transaction to include a fee for creating a (named?) account? What is the motivation behind this? Isn’t that not necessary today? What happens if the implicit account already exists and the intent is to test a basic transfer from one account via a meta transaction?
There was a problem hiding this comment.
Yeah that's unfortunately already what happens today on mainnet, regardless of meta transactions.
I just learned about this this week, which lead me to write this comment on another NEP: near/NEPs#448 (comment)
Others seemed also somewhat surprised. ^^
This test util (which I added very recently) was just wrong / incompatible with implicit accounts.
Motivation: You don't know ahead of time if you need to create it or not but you must inlcude enough gas to cover all cases.
I hope we can get around to fix the implicit account creation flow and make it go through CreateAccount instead of (ab)using the transfer action for it, which would allow dropping the extra cost here. See also near/NEPs#462
| } | ||
|
|
||
| /// Try creating an implicit account with `CreateAction` which is not allowed in | ||
| /// or outside meta transactions and must fail with `OnlyImplicitAccountCreationAllowed`. |
There was a problem hiding this comment.
Observation: isn’t this error variant a misnomer? We’re saying that you can only create implicit accounts where the expectation really is that you can only create named accounts…?
There was a problem hiding this comment.
Yeah, good point, very confusing.
I think the naming wants to say, "you are trying to create a 64 char length account, this is only allowed in the fancy new implicit account creation flow", where "fancy and new" refers back to 2020 and this NEP that introduced the feature: near/NEPs#71
Before that, named accounts were the only type of account.
There was a problem hiding this comment.
As it is confusing for all of us - can we rename error to smth like OnlyNamedAccountCreationAllowed in separate PR? If this is part of protocol, we can try renaming enum name but leave conversion to string as is.
There was a problem hiding this comment.
Agreed. I believe it's not a protocol change to rename this, the borsh representation should be fine and I don't think we store errors as strings anywhere it matters. But the RPC consumers might care, we have had unfortunate surprises around that in the past.
For now I filed this: #8598
There was a problem hiding this comment.
And I added a TODO in the code comment
| } | ||
|
|
||
| /// Try creating an implicit account with `CreateAction` which is not allowed in | ||
| /// or outside meta transactions and must fail with `OnlyImplicitAccountCreationAllowed`. |
There was a problem hiding this comment.
As it is confusing for all of us - can we rename error to smth like OnlyNamedAccountCreationAllowed in separate PR? If this is part of protocol, we can try renaming enum name but leave conversion to string as is.
Integration tests for different ways of creating accounts.
This adds some test utilities for implicit accounts, and it fixes the
meta tx checker functions to also work with implicit accounts, as
they don't use the account name as seed for the access key.