Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Implement sync function of legacy chain handler - Closes #7547 & #7486 #7541

Merged
merged 5 commits into from
Oct 21, 2022
Merged

Implement sync function of legacy chain handler - Closes #7547 & #7486 #7541

merged 5 commits into from
Oct 21, 2022

Conversation

sitetester
Copy link
Contributor

@sitetester sitetester commented Sep 20, 2022 •
edited by shuse2
Loading

What was the problem?

This PR resolves #7547 and resolves #7486

How was it solved?

syncBlocks() function is commented with flow of syncing legacy blocks

How was it tested?

@sitetester sitetester changed the title 7486 implement sync function of legacy chain handler implement sync function of legacy chain handler Sep 20, 2022
@sitetester sitetester changed the title implement sync function of legacy chain handler Implement sync function of legacy chain handler Sep 20, 2022
@sitetester sitetester self-assigned this Sep 20, 2022
@sitetester sitetester changed the base branch from development to feature/7136-support-legacy-blocks-and-transactions September 20, 2022 16:31
ishantiw
ishantiw suggested changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@ishantiw ishantiw left a comment

Choose a reason for hiding this comment

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

Overall approach looks good to me, few comments

framework/src/engine/legacy/legacy_chain_handler.ts Outdated Show resolved Hide resolved
framework/src/engine/legacy/legacy_chain_handler.ts Outdated Show resolved Hide resolved
framework/src/engine/legacy/legacy_chain_handler.ts Outdated Show resolved Hide resolved
framework/src/engine/legacy/legacy_chain_handler.ts Show resolved Hide resolved
framework/src/engine/legacy/legacy_chain_handler.ts Outdated Show resolved Hide resolved
framework/src/engine/legacy/legacy_chain_handler.ts Outdated Show resolved Hide resolved
framework/src/engine/legacy/storage.ts Outdated Show resolved Hide resolved
@sitetester sitetester changed the title Implement sync function of legacy chain handler Implement sync function of legacy chain handler Closes #7547 Sep 29, 2022
@sitetester sitetester changed the title Implement sync function of legacy chain handler Closes #7547 Implement sync function of legacy chain handler - Closes #7547 Sep 29, 2022
@sitetester sitetester changed the title Implement sync function of legacy chain handler - Closes #7547 Implement sync function of legacy chain handler - Closes #7547 & #7486 Sep 29, 2022
@ishantiw ishantiw requested a review from mosmartin October 4, 2022 13:28
@sitetester sitetester force-pushed the 7486-implement-sync-function-of-LegacyChainHandler branch from 9936fb8 to f50581e Compare October 7, 2022 15:44
@sitetester sitetester marked this pull request as ready for review October 11, 2022 08:08
mosmartin
mosmartin reviewed Oct 11, 2022
View reviewed changes
Copy link
Contributor

@mosmartin mosmartin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Besides the open comments, LGTM. I'll have another review once the comments are closed.

ishantiw
ishantiw reviewed Oct 12, 2022
View reviewed changes
Copy link
Contributor

@ishantiw ishantiw 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 to me, please add missing tests and we can finalize the PR

mosmartin
mosmartin approved these changes Oct 13, 2022
View reviewed changes
Copy link
Contributor

@mosmartin mosmartin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Having another look.

@mosmartin mosmartin self-requested a review October 13, 2022 09:29
@sitetester sitetester force-pushed the 7486-implement-sync-function-of-LegacyChainHandler branch from 3abd80e to fd754a8 Compare October 19, 2022 14:27
@sitetester sitetester mentioned this pull request Oct 20, 2022
Closed
ishantiw
ishantiw suggested changes Oct 20, 2022
View reviewed changes
Copy link
Contributor

@ishantiw ishantiw left a comment

Choose a reason for hiding this comment

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

Just one point otherwise lgtm

framework/src/engine/legacy/legacy_chain_handler.ts Outdated Show resolved Hide resolved
@shuse2 shuse2 requested a review from ishantiw October 21, 2022 07:24
ishantiw
ishantiw approved these changes Oct 21, 2022
View reviewed changes
Copy link
Contributor

@ishantiw ishantiw left a comment

Choose a reason for hiding this comment

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

Great job @sitetester 👏

@ishantiw ishantiw merged commit 3e8ad88 into feature/7136-support-legacy-blocks-and-transactions Oct 21, 2022
@ishantiw ishantiw deleted the 7486-implement-sync-function-of-LegacyChainHandler branch October 21, 2022 14:21
@ishantiw ishantiw linked an issue Oct 21, 2022 that may be closed by this pull request
Implement sync method on LegacyChainHandler #7547
Closed
@sitetester sitetester mentioned this pull request Oct 21, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mosmartin mosmartin mosmartin approved these changes

@ishantiw ishantiw ishantiw approved these changes

Assignees

@sitetester sitetester

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement sync method on LegacyChainHandler
3 participants
@sitetester @ishantiw @mosmartin