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

Rename DBFT Settings #3484

Closed
wants to merge 1 commit into from
Closed

Rename DBFT Settings #3484

wants to merge 1 commit into from

Conversation

vncoelho
Copy link
Member

Renaming in the same way as other plugins, such as RpcServerSettings

Copy link
Contributor

@igormcoelho igormcoelho left a comment

Choose a reason for hiding this comment

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

Ok, better naming.

Copy link
Member

@cschuchardt88 cschuchardt88 left a comment

Choose a reason for hiding this comment

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

This may break 3rd-party apps like neo-express. But good job. Changes like this for the better.

Copy link
Contributor

@igormcoelho igormcoelho left a comment

Choose a reason for hiding this comment

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

It seems all other call "Settings".... so maybe keep that way.

@vncoelho
Copy link
Member Author

We are not sure if the change if fully correct, @cschuchardt88.

We just noticed that maybe we need to set a Record to be more compatible.

@cschuchardt88
Copy link
Member

cschuchardt88 commented Sep 11, 2024

Just make a dupe class with DBFTPluginSettings that inherits Settings. Than mark Settings as obsolete. Keeping all parameters with Settings. Than any public properties or places with DBFTPluginSettings.

I think the class constructor is the only place for Settings class. So do above. but just create another constructor like

[Obsolete("use DBFTPluginSettings class")]
public ConsensusContext(NeoSystem neoSystem, Settings settings, Wallet wallet) { }

// new way
public ConsensusContext(NeoSystem neoSystem, DBFTPluginSettings settings, Wallet wallet) { }

Than in 3.8.1 we make Settings Internal or remove it.

@vncoelho
Copy link
Member Author

I think this will not be a necessary change for now, @cschuchardt88

Later we make this change

@vncoelho vncoelho closed this Sep 11, 2024
@vncoelho vncoelho deleted the rename-dbft-settings branch September 11, 2024 00:02
@cschuchardt88
Copy link
Member

I think this will not be a necessary change for now, @cschuchardt88

Later we make this change

We still need this in several plugins.

@vncoelho
Copy link
Member Author

I think this will not be a necessary change for now, @cschuchardt88
Later we make this change

We still need this in several plugins.

I think that we can make the change to all plugins in the same PR, then we organize this correctly!

@vncoelho
Copy link
Member Author

If you want to proceed with this in another PR it will be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants