Skip to content
This repository was archived by the owner on Nov 10, 2025. It is now read-only.

mpt project#658

Merged
shargon merged 9 commits intoneo-project:masterfrom
ZhangTao1596:separate-mpt
Nov 9, 2021
Merged

mpt project#658
shargon merged 9 commits intoneo-project:masterfrom
ZhangTao1596:separate-mpt

Conversation

@ZhangTao1596
Copy link
Collaborator

Close #648

using System.IO;

namespace Neo.Plugins.MPT
namespace Neo.Cryptography.MPT
Copy link
Contributor

Choose a reason for hiding this comment

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

A Neo.Cryptography in the plugin? It is misleading. We have Neo.Cryptography in the neo core already.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree. .NET guidelines specifies "the goal when naming namespaces is creating sufficient clarity for the programmer using the framework to immediately know what the content of the namespace is likely to be". So, I think using Neo.Cryptography.MPT here is fine.

Since the classes in the namespace are all name prefixed with MPT, having MPT in the namespace is as well seems redundant. Neo.Cryptography.MPT.MPTTrie? Do we need to repeat MPT here? Why not just Neo.Cryptography.MPT.MPTTrie?

Note, Neo already has places where namespaces are used across multiple binaries. In particular, all the plugins in this repo are in the Neo.Plugins namespace. We even have multiple types named Neo.Plugins.Settings (ApplicationLogs and RpcServer)

Copy link
Contributor

@Jim8y Jim8y Oct 20, 2021

Choose a reason for hiding this comment

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

The namespace is used to organize code, I don't think it is a good idea to have the same namespace across multiple projects.

Copy link
Member

Choose a reason for hiding this comment

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

I think it could be a distinct namespace

Copy link
Collaborator Author

@ZhangTao1596 ZhangTao1596 Oct 21, 2021

Choose a reason for hiding this comment

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

We have Neo.IO namespace in LevelDBPlugin as well as in core.

Copy link
Collaborator Author

@ZhangTao1596 ZhangTao1596 Oct 21, 2021

Choose a reason for hiding this comment

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

Since the classes in the namespace are all name prefixed with MPT, having MPT in the namespace is as well seems redundant. Neo.Cryptography.MPT.MPTTrie? Do we need to repeat MPT here? Why not just Neo.Cryptography.MPT.MPTTrie?

Does make sense. Neo.Cryptography.MPTTrie? Should we change all class names? They all have MPT prefix.

devhawk
devhawk previously approved these changes Oct 20, 2021
@ZhangTao1596
Copy link
Collaborator Author

@devhawk Names updated.

@ZhangTao1596
Copy link
Collaborator Author

@shargon @devhawk Merge?

shargon
shargon previously approved these changes Oct 28, 2021
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.

Please, wait @erikzhang 's review

@erikzhang
Copy link
Member

Can you fix the ut?

@ZhangTao1596
Copy link
Collaborator Author

ZhangTao1596 commented Nov 3, 2021

Can you fix the ut?

Sure.
Done.

@devhawk devhawk mentioned this pull request Nov 3, 2021
13 tasks
@ZhangTao1596
Copy link
Collaborator Author

@erikzhang @shargon Merge?

@shargon shargon merged commit f8ce79c into neo-project:master Nov 9, 2021
@ZhangTao1596 ZhangTao1596 deleted the separate-mpt branch October 10, 2022 02:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MPTTrie classes should be available in class library independently from StateStore plugin

6 participants