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

Mark the bootstrapped accounts as "seen". #340

Closed
wants to merge 1 commit into from
Closed

Mark the bootstrapped accounts as "seen". #340

wants to merge 1 commit into from

Conversation

andreibancioiu
Copy link

Motivation

Currently, if check:data is ran using a bootstrap_balances.json file, the reconciler does not see the bootstrapped accounts until the process is restarted (when the seen accounts are loaded from balanceStorage). Under these circumstances, the bootstrapped accounts are not reconciled (unless rosetta-cli restarts).

Solution

The proposed solution is to load the bootstrapped accounts from the bootstrap_balances.json file, if such a file is provided. Then, use these accounts (in addition to the ones loaded from balancesStorage) to initialize the reconciler.

Open questions

N / A

@shiatcb
Copy link
Contributor

shiatcb commented Jul 29, 2022

@andreibancioiu are you able to test this change locally and verify it's working and not impact existing workflow?

@@ -218,6 +218,17 @@ func InitializeData(
return nil, fmt.Errorf("%s: unable to get previously seen accounts", err.Error())
}

// We need to mark the bootstrapped accounts as "seen",
// otherwise they won't be reconciled until rosetta-cli restart.
if len(config.Data.BootstrapBalances) > 0 {
Copy link
Contributor

@GeekArthur GeekArthur Jul 29, 2022

Choose a reason for hiding this comment

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

Bootstrap balance should be only used when historical balance is disabled, we may not leave this logic in the main code path to avoid breaking changes for other blockchains with historical balance enabled

Copy link
Author

Choose a reason for hiding this comment

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

Thank you!

If the check:data configuration file does not contain a bootstrap balances file (as it should be the case for blockchains with historical balances enabled, if I understood correctly), then the newly added behavior is inactivated.

However, since historical balances lookup support is always desired and recommended, this PR can be closed, perhaps?

@GeekArthur
Copy link
Contributor

@andreibancioiu Can you provide more context in terms of the specific use case and reproducing steps? I would suggest to open an issue to add those information

@andreibancioiu
Copy link
Author

@shiatcb, @GeekArthur, thank you 🙏

I was able to verify the changes brought by the PR on both flows:

  • with historical balances lookup
  • without historical balances lookup

However, since historical balances lookup support is always desired and recommended, this PR can be closed, perhaps?

@GeekArthur
Copy link
Contributor

Close this as historical balance lookup is supported and it's required to support

@GeekArthur GeekArthur closed this Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants