fix: #2095 follow-ups#2132
Conversation
| /// This wraps the full `AccountId` of a network account, typically extracted | ||
| /// from a `NetworkAccountTarget` attachment. | ||
| #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)] | ||
| pub struct NetworkAccountId(AccountId); |
There was a problem hiding this comment.
We should also remove the proto definition at proto/proto/internal/store.proto:253-256.
| /// should pre-filter to post-deployment, public-account ids; `Ok(())` on empty. | ||
| async fn reject_if_any_network_accounts( | ||
| &self, | ||
| candidate_ids: Vec<proto::account::AccountId>, |
There was a problem hiding this comment.
It may be better to take domain type AccountId here, then convert internally. So function callers (which are domain type) don't need to be aware of the proto parallels.
juan518munoz
left a comment
juan518munoz
left a comment
There was a problem hiding this comment.
Besides the small comments it looks ok IMO. Note that the las two commits (removing details_commitment from store and adding netwrok account cache to rpc service) are of my own, so my approval could be consider disregardable in those sections.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a couple of non-blocking comments inline.
| 6, | ||
| Felt::new_unchecked(10_000_000_000), | ||
| Felt::new_unchecked(0), | ||
| Felt::from_u64(10_000_000_000u64), |
There was a problem hiding this comment.
nit: I'd use Felt::new_unchecked() and then we don't need PrimeCharacteristicRing import above (the effect will be the same).
| // Inherit the classification when the account already exists; otherwise classify it once at | ||
| // creation based on the new state. | ||
| let network_account_type = match existing { |
There was a problem hiding this comment.
Not for this PR, but somewhere (probably much further upstream), we should try to detect if the delta is trying to introduce (or update) the storage slot that identifies network accounts. In case such a storage slot is being added to an existing account, we should throw an error (accounts cannot become network accounts after creation), if it is an update, then NTX builder will need to update the set of notes that the account is able to process.
Let's create an issue for this (unless we have one already).
This reverts commit 69aa665.
3 participants
AreNetworkAccountstoFilterNetworkAccounts(link)NetworkAccountIdSubsetintoaccount.AccountIdList(link)state::are_network_accountstofilter_network_accounts(link) andrpc_api::are_network_accountstofilter_network_accounts(link)NetworkAccountIdnewtype wrapper (link, link, link, link)try_into().context()for MMR forest size with a meaningful message (link)proto::note::NoteHeaderround-trip by usingdetails_commitmentinstead ofnote_id(link); also adds a test for thisstorage_modetoaccount_typeandStorageMode->AccountTypeConfig(link, link, link, link)SAFETYcomment toraw_sql_to_nonce(link)rpc::server::api(link)details_commitmentfield fromStore(link)RpcService(link)