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

feat: adds withdrawal finalizing logic. #56

Merged
merged 38 commits into from
Jul 26, 2023

Conversation

montekki
Copy link
Contributor

No description provided.

@montekki
Copy link
Contributor Author

40851b2 adds initial implementation of finalizer that migrates data from watcher table into finalization_data table together with withdrawal finalization paramethers:

Screenshot 2023-07-19 at 16 18 03

@montekki montekki marked this pull request as ready for review July 24, 2023 06:21
config.batch_finalization_gas_limit,
);

let finalizer = finalizer::Finalizer::new(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now it became a bit confusing:
We have WithdrawalFinalizer and Finalizer

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's choose the proper names for all components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I wanted to do rename in another patch not to bloat this one

let is_finalized = self
.is_withdrawal_finalized(event.tx_hash, index, event.token)
.await?;
let is_finalized = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is it always false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This field is being retied as all relevant info now lives in finalization_data table. Hoever I believe that we cannot delete this field since it would be breaking?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, I think in terms of the stage of the project we can.
We can't afford a nonworking state keeper.
But here it's better to have less legacy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

column removed in d667fc3

client/src/lib.rs Show resolved Hide resolved
finalizer/src/accumulator.rs Outdated Show resolved Hide resolved
finalizer/src/lib.rs Outdated Show resolved Hide resolved

// process withdrawals that have been predicted as unsuccessful.
//
// there may be two reasons for such predictions:
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's just the main reasons.
But it also possible, that ERC20 contract reject those txs for some internal reasons

finalizer/src/lib.rs Outdated Show resolved Hide resolved
finalizer/src/lib.rs Outdated Show resolved Hide resolved
storage/src/lib.rs Show resolved Hide resolved
finalizer/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Deniallugo Deniallugo left a comment

Choose a reason for hiding this comment

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

Micro nit

where
M2: ZksyncMiddleware + 'static,
{
let migrator_handle = tokio::spawn(params_fetcher_loop(self.pgpool.clone(), middleware));
Copy link
Collaborator

Choose a reason for hiding this comment

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

still migrator

@montekki montekki merged commit 5297c02 into main Jul 26, 2023
@montekki montekki deleted the fvs-pla-410-finalization-implementation branch July 26, 2023 15:02
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.

3 participants