Skip to content
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

Improve migration process #80

Merged
merged 10 commits into from
Apr 28, 2024
Merged

Improve migration process #80

merged 10 commits into from
Apr 28, 2024

Conversation

karim-en
Copy link
Collaborator

@karim-en karim-en commented Feb 21, 2024

Description

This PR improves the migration process and makes it more trustless by retrieving the balances on-chain instead of off-chain.

Changes:

Performance / NEAR gas cost considerations

Testing

How should this be reviewed

Additional information

@karim-en karim-en requested review from mrLSD and aleksuss February 21, 2024 01:16
@karim-en karim-en marked this pull request as ready for review March 29, 2024 21:43
@karim-en karim-en requested review from kiseln and removed request for aleksuss March 29, 2024 21:43
@karim-en karim-en self-assigned this Mar 29, 2024
@karim-en karim-en requested a review from olga24912 April 1, 2024 14:29
#[cfg(feature = "integration-test")]
#[result_serializer(borsh)]
#[must_use]
pub fn ft_balances_of(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we don't create a real ft_balance_of function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we are going to migrate accounts batches, so we need to get balances for multiple accounts at once to reduce the time that is needed for migration and gas usage.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question was not about that. This function exists only if the integration-test feature turns on. But why don't we just support this function in the eth-connector contract? Why do we need to create a fake one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function returns mock values for tests and simulates the Aurora engine contract.
It is the same as the verify_log_entry_in_bound that simulates the Prover contract.

eth-connector/src/lib.rs Show resolved Hide resolved
Copy link
Collaborator

@aleksuss aleksuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good.

if let Some(previous_balance) = self.ft.accounts.insert(account, amount) {
self.ft.total_supply -= previous_balance;
}
self.ft.total_supply += amount;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move the line self.ft.total_supply += amount; to the beginning of the loop to prevent overflow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logically, it is impossible to get overflow(underflow) here. If we got it, then it means that we did something wrong, and the migration process should be stopped. The contract will panic because the flag overflow-checks is enabled.

@karim-en karim-en merged commit abc7952 into master Apr 28, 2024
6 checks passed
@karim-en karim-en deleted the improve-migration branch April 28, 2024 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants