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

[Neo Core Style] update smartcontract struct and name #3488

Closed
wants to merge 8 commits into from

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented Sep 12, 2024

Description

Currently different group of files are put together under the network payload and smart contract, making it hard to understand the payload structure. And same files are not named properly, such as Conflicts is actually ConflictAttribute. Thus having this pr that update the files position to properly group them, and also rename some class to properly reflect its purpose.

In this pr, no logic has being changed.

Fixes # (issue)

Type of change

  • Optimization (the change is only an optimization)
  • Style (the change is only a code style for better maintenance or standard purpose)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Test Configuration:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

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.

Maybe we should also move the namespaces, have some files with the namespace according to the folder, and others don't seems weird

@Jim8y
Copy link
Contributor Author

Jim8y commented Sep 12, 2024

Maybe we should also move the namespaces, have some files with the namespace according to the folder, and others don't seems weird

Namespace has being a problem in every core project and almost every file, you can have a seperate pr to focus on that issue if you think it should be fixed.

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.

I'm not against the pr, but it changes a public class, so maybe we can also remove some obsolete calls because we will need a new version

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

No functional changes, but is neo-devpack or other dependent tools affected by this renaming? If so, then I agree with @shargon, some transition period is needed. If no one uses these classes except the core, then it may be merged as is.

@cschuchardt88
Copy link
Member

cschuchardt88 commented Sep 14, 2024

The naming is still bad ConflictAttribute name suggests that its uses the Attribute class in dotnet or an attribute for a field, property or method. Putting it under a namespace named Neo.Network.P2P.Payloads.Witness.Attributes with the name Conflict is more suitable. Plus seeing how the class is still public, no need to make it obsolete. Developers know when from version to version code, namespace, methods and etc can change. I see this being no part. Because you have to nuget new version and compile. Compiling will fail yes, but doesn't block or stop them from changing it.

@Jim8y
Copy link
Contributor Author

Jim8y commented Sep 14, 2024

No functional changes, but is neo-devpack or other dependent tools affected by this renaming? If so, then I agree with @shargon, some transition period is needed. If no one uses these classes except the core, then it may be merged as is.

@AnnaShaleva @shargon can you please check if the update works. Just add them back but mark as obsolete.

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

@@ -29,7 +31,7 @@ public class NotValidBefore : TransactionAttribute
public override bool AllowMultiple => false;

public override int Size => base.Size +
sizeof(uint); // Height.
sizeof(uint); // Height.
Copy link
Member

Choose a reason for hiding this comment

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

Useless change.

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 think this may cause by my dotnet format.

Copy link
Member

Choose a reason for hiding this comment

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

Not fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please focus on functionality.

using System.IO;

namespace Neo.Network.P2P.Payloads
{
[Obsolete("Use ConflictsAttribute instead")]
public class Conflicts : TransactionAttribute
Copy link
Member

@shargon shargon Sep 19, 2024

Choose a reason for hiding this comment

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

Before remove maybe is easier to mantain if we share the code

Suggested change
public class Conflicts : TransactionAttribute
public class Conflicts : ConflictsAttribute

Copy link
Member

Choose a reason for hiding this comment

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

@shargon thats what i said here #3488 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before remove maybe is easier to mantain if we share the code

But code is already there, keep this pr this way as much easier isn't it~~~~

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Please, take care of src/Neo/Builders/TransactionAttributesBuilder.cs and benchmarks/Neo.VM.Benchmarks/Benchmarks.Types.cs, because these classes use obsolete attributes; we need to replace them with the new ones.

@@ -29,7 +31,7 @@ public class NotValidBefore : TransactionAttribute
public override bool AllowMultiple => false;

public override int Size => base.Size +
sizeof(uint); // Height.
sizeof(uint); // Height.
Copy link
Member

Choose a reason for hiding this comment

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

Not fixed.

@Jim8y
Copy link
Contributor Author

Jim8y commented Oct 1, 2024

too many non-functional suggestions that make it hard to maintain this pr, thus close to save my effort.

@Jim8y Jim8y closed this Oct 1, 2024
@Jim8y Jim8y deleted the update-structure branch October 1, 2024 10:35
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.

4 participants