Storage layer for multiple accounts #3059
Conversation
LXIF
changed the title
LXIF
changed the title
lmuntaner
force-pushed
the
andri/macid-storage
branch
from
314e465 to
a51af68
Compare
lmuntaner
force-pushed
the
andri/macid-storage
branch
2 times, most recently
from
5cb9f12 to
063ff24
Compare
lmuntaner
changed the title
lmuntaner
force-pushed
the
andri/macid-storage
branch
from
1fe3545 to
55a65aa
Compare
|
@sea-snake this is finally ready for review. On the other hand, it includes all the planned functionality in the storage layer. I finally decided to create a different struct for each method. Here is the comparison table: This can be improved, but adding extra parameters or making them optional when we don't want them optional made the methods more complex and I think those should be handled gracefully in the business logic, not at the storage layer. |
There was a problem hiding this comment.
I don't see a account limit and/or counter that keeps track of the number of stored default/additional accounts.
Given that we'd need to read all account references (with a range) every time we'd want to check this, should we consider two separate counters in stable memory for this?
| /// If the account number doesn't esist, returns a default Account. | ||
| /// If the account number exists but the account doesn't exist, returns None. | ||
| /// If the account exists, returns it as Account. |
There was a problem hiding this comment.
Comment didn't match implementation, see below suggestion:
| /// If the account number doesn't esist, returns a default Account. | |
| /// If the account number exists but the account doesn't exist, returns None. | |
| /// If the account exists, returns it as Account. | |
| /// If the application doesn't exist, returns a default Account. | |
| /// If the application exists but the account doesn't exist, returns None. | |
| /// If the account exists, returns it as Account. |
There was a problem hiding this comment.
Why do you say it doesn't match? This is the read_account, which checks the account_number. If None, returns a default account. Otherwise, it looks it up in memory.
There was a problem hiding this comment.
/// Line 933-942: If the application doesn't exist, returns a default Account.
/// Line 944: If the application exists but the account doesn't exist, returns None.
/// Line 945-950: If the account exists, returns it as Account.
88dde91 to
e83a539
Compare
|
@sea-snake changes done and passing pipeline green. Please take a look, thanks! |
src/internet_identity/src/storage.rs
Outdated
| vec![default_account_reference, additional_account_reference].into(), | ||
| ); | ||
| // Two new account references were created. | ||
| self.update_counters(app_num, anchor_number, AccountType::AccountReference)?; |
There was a problem hiding this comment.
We should always increase counters before inserting, increasing it without inserting an account due to an error is less critical than inserting an account and not increasing it due to an error.
| for storable_ref in storable_refs_vec { | ||
| all_accounts.push(AccountReference { | ||
| account_number: storable_ref.account_number, | ||
| last_used: storable_ref.last_used, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Nit: you can probably use flat_map to achieve the same thing
| return Err(StorageError::MissingAccount { | ||
| anchor_number: params.anchor_number, | ||
| name: params.name.clone(), | ||
| }); |
There was a problem hiding this comment.
At this exit point, is it OK that some mutations have happened? (lookup_or_insert_application_number_with_origin and update_counters
There was a problem hiding this comment.
Yes, we will be adding a check to fix the counters in case they run out of sync.
And one more application isn't so bad either, it's just a bit more memory.
| for r_mut in refs_vec.iter_mut() { | ||
| if r_mut.account_number.is_none() { | ||
| // Found the default account reference. | ||
| r_mut.account_number = Some(new_account_number); | ||
| found_and_updated = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
If you do .iter_mut().find(...), I think it will be more apparent that no mutations have happened in the error case
Motivation
This is a preliminary draft of the Multi-ACcount IDentity storage implementation.
Changes
A lot! Created the storage structures and several structs and access methods and updated some of the lookup methods and such. This needs to be refactored though, and is definitely incomplete.
Tests
Added one unit test, which runs fine.