-
Notifications
You must be signed in to change notification settings - Fork 6
test: FI-1857: Test upgrade and reinstall with index that handles burn and mint fees #167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: FI-1857: Test upgrade and reinstall with index that handles burn and mint fees #167
Conversation
cycles-ledger/tests/tests.rs
Outdated
| let from_balance = self.expected_ledger_balances.entry(from).or_insert(0); | ||
| *from_balance -= amount_with_fee; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't or_insert(0) a bit weird here because the next line would then panic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! I reworked the code for adding/subtracting to/from the expected balances, hopefully it's more solid and clearer now.
cycles-ledger/tests/tests.rs
Outdated
| let from_index_balance = self.expected_index_balances.entry(from).or_insert(0); | ||
| *from_index_balance -= amount_with_fee; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
cycles-ledger/tests/tests.rs
Outdated
| index_version: IndexVersion, | ||
| ) { | ||
| env.deposit(account1, DEPOSIT_AMOUNT_ACCOUNT_1 + fee, None); | ||
| balances_verifier.record_deposit(env, account1, DEPOSIT_AMOUNT_ACCOUNT_1 + fee); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't the deposit value simply DEPOSIT_AMOUNT_ACCOUNT_1 (i.e., why is the fee added on top)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, it's not really necessary. I simplified this as suggested.
| reinstalled_balances_verifier.sync_and_verify_index(&env); | ||
|
|
||
| perform_transactions(&mut env); | ||
| perform_transactions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this function called at the end of the test function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sync_and_verify_index checks that we didn't e.g., lose/corrupt any balances as part of the upgrade/reinstall, and the perform_transactions calls afterward verifies that the upgraded/reinstalled index still pulls transactions from the ledger and updates the balances correctly. If we e.g., introduced a bug that would fail to start the index timer for retrieving ledger transactions in certain scenarios, or corrupted balances when processing new transactions, then we might only catch it in perform_transactions.
Co-authored-by: Thomas Locher <[email protected]>
|
Thanks for the review, @THLO! I believe I've addressed all your comments, could you please take another look? |
Test upgrade and reinstall with the currently installed version, and the latest icrc-ledger-suite-2025-10-27 (pre-)release version of the index canister.