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

Replace rounds.generateDelegateList by dpos.getRoundDelegates - Closes #4152 #4154

Conversation

lsilvs
Copy link
Contributor

@lsilvs lsilvs commented Aug 27, 2019

What was the problem?

rounds.generateDelegateList still being used in the codebase

How did I solve it?

By replacing every use of rounds.generateDelegateList by dpos.getRoundDelegates.

During the replacement, it was identified an issue with dpos.getRoundDelegates -> #4147
Because of this issue, two tests were skipped and should be unskipped as part of its fix.

How to manually test it?

Green build
Should sync on testnet at least up to height 10000

Review checklist

Copy link
Contributor

@michielmulders michielmulders left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@yatki yatki left a comment

Choose a reason for hiding this comment

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

LGTM in general. Just two minor comments and a question :D

const nextForger = delegateList[(slot + offset) % ACTIVE_DELEGATES];
return seriesCb(nextForger);
});
library.modules.dpos.getRoundDelegates(round).then(delegateList => {
Copy link
Contributor

Choose a reason for hiding this comment

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

if it doesn't require a big refactor, maybe we can await here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would suggest creating a new issue to replace use of callbacks in this file and use async/await consistently.

framework/test/mocha/integration/rounds.js Outdated Show resolved Hide resolved
framework/src/modules/chain/rounds/rounds.js Show resolved Hide resolved
@lsilvs lsilvs requested a review from yatki August 27, 2019 13:00
@shuse2 shuse2 merged commit 766d922 into feature/introduce_bft_consensus Aug 27, 2019
@shuse2 shuse2 deleted the 4152-replace-rounds-generateDelegateList branch August 27, 2019 14:06
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.

None yet

4 participants