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.

Sign up for GitHub

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

Jump to bottom

[DBFTPlugin] Dbft constructor #545

Merged
merged 20 commits into from
Mar 15, 2021
Merged

[DBFTPlugin] Dbft constructor #545

merged 20 commits into from
Mar 15, 2021

Conversation

ZhangTao1596
Copy link
Collaborator

Close #455 #543 #544

Changes:

  • Avoid using static
  • Provide settings in constructors of ConsensusService and ConsensusContext.

@erikzhang @shargon @devhawk

@ZhangTao1596 ZhangTao1596 changed the title Dbft constructor [DBFTPlugin] Dbft constructor Mar 10, 2021
shargon
shargon previously approved these changes Mar 10, 2021
View reviewed changes
shargon
shargon previously approved these changes Mar 10, 2021
View reviewed changes
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

Wait for @superboyiii tests

devhawk
devhawk reviewed Mar 10, 2021
View reviewed changes
src/DBFTPlugin/Settings.cs Outdated Show resolved Hide resolved
devhawk
devhawk suggested changes Mar 10, 2021
View reviewed changes
Copy link
Contributor

@devhawk devhawk left a comment

Choose a reason for hiding this comment

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

Settings.Default should be eliminated

@shargon shargon requested a review from devhawk March 10, 2021 19:22
devhawk
devhawk suggested changes Mar 11, 2021
View reviewed changes
Copy link
Contributor

@devhawk devhawk left a comment

Choose a reason for hiding this comment

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

I still need a mechanism to pass custom settings to DBFTPlugin. I suggested adding an optional settings parameter to DBFTPlugin.Start method

@superboyiii
Copy link
Member

Test PASS
Works well on mine.
image

superboyiii
superboyiii previously approved these changes Mar 12, 2021
View reviewed changes
devhawk
devhawk reviewed Mar 12, 2021
View reviewed changes
src/DBFTPlugin/DBFTPlugin.cs Outdated
{
if (started) return;
started = true;
if (settings != null) this.settings = settings;
consensus = neoSystem.ActorSystem.ActorOf(ConsensusService.Props(neoSystem, settings, wallet));
Copy link
Contributor

@devhawk devhawk Mar 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

you're missing a this on line 61. when there's a method parameter and class field with the same identitifer (aka settings) you need to use this. to distinguish between them. The way the code is now, the settings passed to ConsensusService.Props will always be the parameter settings, even if it's null.

consensus = neoSystem.ActorSystem.ActorOf(ConsensusService.Props(neoSystem, this.settings, wallet));

devhawk
devhawk reviewed Mar 12, 2021
View reviewed changes
Copy link
Contributor

@devhawk devhawk left a comment

Choose a reason for hiding this comment

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

Fix the missing this. in Start and it looks good to me

devhawk
devhawk previously approved these changes Mar 12, 2021
View reviewed changes
@shargon shargon dismissed stale reviews from devhawk and superboyiii via 34ed537 March 12, 2021 10:38
devhawk
devhawk suggested changes Mar 13, 2021
View reviewed changes
Copy link
Contributor

@devhawk devhawk left a comment

Choose a reason for hiding this comment

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

still waiting on the this. fix in Start

@ZhangTao1596
Copy link
Collaborator Author

still waiting on the this. fix in Start

Done.

shargon
shargon reviewed Mar 13, 2021
View reviewed changes
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

Tested?

erikzhang
erikzhang previously approved these changes Mar 13, 2021
View reviewed changes
@erikzhang
Copy link
Member

@devhawk Can you review it again?

@erikzhang
Copy link
Member

Merge?

@superboyiii superboyiii merged commit adffb96 into neo-project:master Mar 15, 2021
@ZhangTao1596 ZhangTao1596 deleted the dbft-constructor branch March 15, 2021 02:52
joeqian10 pushed a commit to joeqian10/neo-modules that referenced this pull request Apr 7, 2021
@ZhangTao1596 @shargon
@erikzhang @joeqian10
[DBFTPlugin] Dbft constructor (neo-project#545)
fcc7d6c 
* provide settings in dbft constructor

* remove static

* rename

* Erik's feedback

* original RecoveryMessage.Deserialize with extra check

* Remove Settings.Default

* message type read once

* Harry's suggestion

* Revert dummy changes

* this

* add verify

* Clean enter

* Remove ()

* Update

* use byte.MaxValue

* Remove cast

Co-authored-by: Shargon <shargon@gmail.com>
Co-authored-by: Erik Zhang <erik@neo.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikzhang erikzhang erikzhang approved these changes

@shargon shargon shargon approved these changes

@devhawk devhawk devhawk approved these changes

@superboyiii superboyiii superboyiii left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move ConsensusPayload processing methods to ConsensusContext
5 participants
@ZhangTao1596 @superboyiii @erikzhang @devhawk @shargon