Skip to content

Commit

Permalink
NNS1-2905: Remove transactions fields from accounts (step 2) (#4836)
Browse files Browse the repository at this point in the history 
This PR should only be merged after we have a release on mainnet
containing #4800.

The commit currently on mainnet:
```
$ dfx canister metadata nns-dapp git_commit_id --network mainnet
581eb32
```
That
[commit](581eb32)
is from May 15th, well after PR #4800, which was merged May 10th.

# Motivation

This is the follow-up to #4800.
In the previous PR we removed the transactions fields but still encoded
to stable memory including the fields.
In this PR we stop encoding the old fields completely.
There will still be accounts in stable memory with the fields but they
will be ignored when reading and removed when writing.
We have not yet decided if we also want to do a migration round to
remove all the fields from stable memory.

# Changes

1. Stop converting `Account` to `OldAccount` before encoding it to
stable memory.

# Tests

1. `upgrade-downgrade-test` passed.
2. Manually tested upgrading and downgrading while creating new
subaccounts before, in between and after.

# Todos

- [x] Add entry to changelog (if necessary).
  • Loading branch information
@dskloetd
dskloetd authored May 21, 2024
1 parent 7a9f74e commit ed2f186
Show file tree
Hide file tree
Showing 3 changed files with 3 additions and 147 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG-Nns-Dapp-unreleased.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ proposal is successful, the changes it released will be moved from this file to

#### Removed

* Stop writing account transactions to stable memory.

#### Fixed

* Rendering tokens with fewer than 8 decimals.
Expand Down
79 changes: 1 addition & 78 deletions rs/backend/src/accounts_store.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ use schema::{

use self::schema::SchemaLabel;

type TransactionIndex = u64;

/// The data migration is more complicated if there are too many accounts. With below this many
/// accounts we avoid some complications.
const PRE_MIGRATION_LIMIT: u64 = 300_000;
Expand Down Expand Up @@ -162,102 +160,27 @@ pub struct Account {
impl Storable for Account {
const BOUND: Bound = Bound::Unbounded;
fn to_bytes(&self) -> Cow<'_, [u8]> {
// Encode Account in a way that can still be restored if we need to
// roll back the release.
let old_account = OldAccount::from(self.clone());
candid::encode_one(old_account)
.expect("Failed to serialize account")
.into()
// TODO(NNS1-2905): Restore direct encoding once OldAccount is deleted.
// candid::encode_one(self).expect("Failed to serialize account").into()
candid::encode_one(self).expect("Failed to serialize account").into()
}
fn from_bytes(bytes: Cow<'_, [u8]>) -> Self {
candid::decode_one(&bytes).expect("Failed to parse account from store.")
}
}

// TODO(NNS1-2905): Delete this after it has been on mainnet for at least 1 release.
#[derive(CandidType, Deserialize, Debug, Eq, PartialEq, Clone)]
struct OldAccount {
principal: Option<PrincipalId>,
account_identifier: AccountIdentifier,
default_account_transactions: Vec<TransactionIndex>,
sub_accounts: HashMap<u8, OldNamedSubAccount>,
hardware_wallet_accounts: Vec<OldNamedHardwareWalletAccount>,
canisters: Vec<NamedCanister>,
}

impl From<Account> for OldAccount {
fn from(account: Account) -> Self {
OldAccount {
principal: account.principal,
account_identifier: account.account_identifier,
default_account_transactions: Vec::new(),
sub_accounts: account
.sub_accounts
.into_iter()
.map(|(id, sa)| (id, sa.into()))
.collect(),
hardware_wallet_accounts: account
.hardware_wallet_accounts
.into_iter()
.map(NamedHardwareWalletAccount::into)
.collect(),
canisters: account.canisters,
}
}
}

#[derive(CandidType, Deserialize, Debug, Eq, PartialEq, Clone)]
struct NamedSubAccount {
name: String,
account_identifier: AccountIdentifier,
// transactions: Do not reuse this field. There are still accounts in stable memory with this unused field.
}

// TODO(NNS1-2905): Delete this after it has been on mainnet for at least 1 release.
#[derive(CandidType, Deserialize, Debug, Eq, PartialEq, Clone)]
struct OldNamedSubAccount {
name: String,
account_identifier: AccountIdentifier,
transactions: Vec<TransactionIndex>,
}

impl From<NamedSubAccount> for OldNamedSubAccount {
fn from(sub_account: NamedSubAccount) -> Self {
OldNamedSubAccount {
name: sub_account.name,
account_identifier: sub_account.account_identifier,
transactions: Vec::new(),
}
}
}

#[derive(CandidType, Deserialize, Debug, Eq, PartialEq, Clone)]
struct NamedHardwareWalletAccount {
name: String,
principal: PrincipalId,
// transactions: Do not reuse this field. There are still accounts in stable memor with this unused field.
}

// TODO(NNS1-2905): Delete this after it has been on mainnet for at least 1 release.
#[derive(CandidType, Deserialize, Debug, Eq, PartialEq, Clone)]
struct OldNamedHardwareWalletAccount {
name: String,
principal: PrincipalId,
transactions: Vec<TransactionIndex>,
}

impl From<NamedHardwareWalletAccount> for OldNamedHardwareWalletAccount {
fn from(hw_account: NamedHardwareWalletAccount) -> Self {
OldNamedHardwareWalletAccount {
name: hw_account.name,
principal: hw_account.principal,
transactions: Vec::new(),
}
}
}

#[derive(CandidType, Deserialize, Clone, Debug, PartialEq, Eq)]
pub struct NamedCanister {
name: String,
Expand Down
69 changes: 0 additions & 69 deletions rs/backend/src/accounts_store/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1075,73 +1075,4 @@ fn accounts_should_implement_storable() {
assert_eq!(account, parsed);
}

fn create_new_test_account_for_encoding() -> Account {
let principal = PrincipalId::from_str(TEST_ACCOUNT_1).unwrap();
let account_identifier = AccountIdentifier::from(principal);

let sub_principal = PrincipalId::from_str(TEST_ACCOUNT_2).unwrap();
let sub_account_identifier = AccountIdentifier::from(sub_principal);

let hw_principal = PrincipalId::from_str(TEST_ACCOUNT_3).unwrap();

let new_sub_account = NamedSubAccount {
name: "Sub1".to_string(),
account_identifier: sub_account_identifier,
};

let mut new_sub_accounts = HashMap::new();
new_sub_accounts.insert(1, new_sub_account);

let new_hw_account = NamedHardwareWalletAccount {
name: "HW1".to_string(),
principal: hw_principal,
};

Account {
principal: Some(principal),
account_identifier,
sub_accounts: new_sub_accounts,
hardware_wallet_accounts: vec![new_hw_account],
canisters: vec![],
}
}

#[test]
fn old_account_can_be_decoded_to_new_account() {
// Test that after upgrading to the next release, we are able to decode the
// old accounts from stable memory.
let expected_new_account = create_new_test_account_for_encoding();
let old_account = OldAccount::from(expected_new_account.clone());

let bytes = candid::encode_one(old_account).unwrap();
let decoded_new_account = Account::from_bytes(Cow::Owned(bytes));

assert_eq!(decoded_new_account, expected_new_account);
}

#[test]
fn new_account_can_be_decoded_to_new_account() {
// Test that after upgrading to the next release after the next release, we
// are able to decode the new accounts that were encoded as old accounts.
let expected_new_account = create_new_test_account_for_encoding();

let bytes = expected_new_account.to_bytes();
let decoded_new_account = Account::from_bytes(bytes);

assert_eq!(decoded_new_account, expected_new_account);
}

#[test]
fn new_account_can_be_decoded_to_old_account() {
// Test that, in case we need to roll back after the next release, the
// previous version is able to decode accounts written by the next version.
let new_account = create_new_test_account_for_encoding();
let expected_old_account = OldAccount::from(new_account.clone());

let bytes = new_account.to_bytes();
let decoded_old_account: OldAccount = candid::decode_one(&bytes.into_owned()).unwrap();

assert_eq!(decoded_old_account, expected_old_account);
}

crate::accounts_store::schema::tests::test_accounts_db!(AccountsStore::default());

0 comments on commit ed2f186

Please sign in to comment.