Implement RPC errors proposal #156

[Jim8y]

https://github.com/neo-project/neo-modules/issues/728 https://github.com/neo-project/neo-modules/issues/814

• [x] https://github.com/neo-project/neo-modules/pull/873

[Jim8y]

@AnnaShaleva Please review

[shargon]

@Liaojinghui if you unify the namespace and fix the unit test we can merge it.

[Jim8y]

@Liaojinghui if you unify the namespace and fix the unit test we can merge it.

I need to talk with you about this pr since it changes the error code, do you think it is ok to merge it? This is inconsistent change.

[shargon]

@Liaojinghui if you unify the namespace and fix the unit test we can merge it.

I need to talk with you about this pr since it changes the error code, do you think it is ok to merge it? This is inconsistent change.

It will unify the error codes, I can't see the problem.

[Jim8y]

@Liaojinghui if you unify the namespace and fix the unit test we can merge it.

I need to talk with you about this pr since it changes the error code, do you think it is ok to merge it? This is inconsistent change.

It will unify the error codes, I can't see the problem.

Fair enough, I will work on it

[Jim8y]

@roman-khimov will need you to define some new error code for the canceltx command. The existing error code are like randomly assigned. As to the format, i run dotnet format everytime i commit.

[AnnaShaleva]

@roman-khimov will need you to define some new error code for the canceltx command.

I don't think we need a separate error code. canceltx creates a simple transaction with Conflicts attribute, it uses a standard transaction relay mechanism, and the error codes should be the same as for the sendrawtransaction. When it comes to NeoGo implementation, we have a bit different CLI scheme and we'll use the standard sendrawtransaction RPC call and send the conflicting transaction from NeoGo CLI to the provided RPC node endpoint (it will be done within the scope of https://github.com/nspcc-dev/neo-go/issues/3151). So I'd say that the error codes will be the same as described at https://github.com/neo-project/proposals/pull/156/files#diff-2b5f7c12a23f7dbe4cb46bbf4be6936882f8e0f0b3a4db9d8c58eb294b02e6edR288-R289.

@Liaojinghui, does this comment answer your question?

[Jim8y]

A good reason please, instead of I dont like it.

• Does this mean neo will not accept any new coding standard in the future?

• Will neo solve all coding style issue in the existing code base?

• What exactly are the coding style?

• Why there is no such style requirement before for so many years, if we are going to have one, who decides?

• Why not other more general coding style, you dont care about name style in the past, yet now you care about namespace style, why?

• Why new language standard can be a coding style issue, how severe it can be?

• Should we stop using new dotnet version?

We need a reason for this, like even dotnet runtime itself adopts new programming coding standard, i dont see any chaos.

[AnnaShaleva]

@Liaojinghui, if some third opinion is still needed for the coding standard question, then I support keeping namespace style consisting with the existing code base for the current PR. And later we can create a separate issue to upgrade the coding conventions in the whole project to match dotnet 7.0. To me, it's a good practice to always keep the style unified with the existing codebase and, after each new dotnet/language release, make the atomic updates.

In NeoGo we use the following pipeline: once the new version of Go language is released, we create an issue to update the whole codebase and to apply new features of this version to the whole project. After that we create a set of PRs that apply these new language features to the project. Note, that these upgrades are always represented as a separate PRs that never change the code logic itself, these PRs just apply new language features. See for example our upcoming Go 1.21 upgrade checklist: https://github.com/nspcc-dev/neo-go/issues/3089. After that the updated changes may even be tested in NeoBench to ensure that new Go runtime doesn't affect the node performance (there was the case like that with Go 1.18 IINM). This is the way of migration to the new language standard or new version of the platform that I appreciate.

I'm against of changing the codebase format in unrelated PRs that solve other issues. It makes PRs harder to review, it distracts the attention from the PR's main goal, it makes PRs harder to port, it makes git history more complicated and it makes life harder in cases when you need to revert commits or to test the performance of new feature.

I may answer to a couple of questions from your list:

Does this mean neo will not accept any new coding standard in the future?

It definitely does not. We should adopt new coding standards, but it may be done via separate designated issues and PRs.

Will neo solve all coding style issue in the existing code base?

It's one of the directions that should be added to https://github.com/neo-project/neo/issues/2949. It's a good way of future development, and we need to unify the whole codebase, but this task is less prioritized than bugs fixing and core protocol support.

Why there is no such style requirement before for so many years, if we are going to have one, who decides?

We probably need to create a separate issue for standartization and discuss there all together. There should be at least basic conventions that will be fixed on the org-level documentation, see e.g. https://github.com/nspcc-dev/.github/blob/master/project-management.md. Although nspcc-dev also doesn't have the unified code style documentation, but I consider it's good to have it in future.

Why not other more general coding style, you dont care about name style in the past, yet now you care about namespace style, why?

Maybe it's time to change the project structure and make it a little bit better. Our priority was to deliver new features, and now we probably have some more time to think not only about new features, but also about our codebase appearance. Good and unified coding style positively affects the development process and sometimes even helps to avoid bugs.

Should we stop using new dotnet version?

I don't think so. But we need a rough, unified and atomic migration process that won't affect the rest of the development tasks.

[cschuchardt88]

Does this mean neo will not accept any new coding standard in the future?

Once one is set in stone. I would say it would be hard to change.

Will neo solve all coding style issue in the existing code base?

Yes! That's what .editorconfig is for.

What exactly are the coding style?

https://github.com/neo-project/neo-express/blob/master/.editorconfig is a good start.

Why there is no such style requirement before for so many years, if we are going to have one, who decides?

I think we should follow the c# coding standards that have been implemented for 2 decades that every c# developer should be using. Me personally after +20 years of c# development i dont expect anything to be any different.

Why not other more general coding style, you dont care about name style in the past, yet now you care about namespace style, why?

c# sets this standard and thats what other c# developers like myself expect when coming to a project. (Thats uses c#)

Why new language standard can be a coding style issue, how severe it can be?

See above answer.

Should we stop using new dotnet version?

Yes, unless we need the new features it offers.

[Jim8y]

Then I follow and obey. With just a little clearance:

I'm against of changing the codebase format in unrelated PRs that solve other issues.

@AnnaShaleva When we switch to dotnet7.0, IDE generate new files directly with the new coding style. I did not mean to change or update anything.

[Jim8y]

Good to review now with the traditional namespace style as suggested. @shargon

Can not increase the coverage since the rpcserver test project is empty.

[AnnaShaleva]

@AnnaShaleva When we switch to dotnet7.0, IDE generate new files directly with the new coding style. I did not mean to change or update anything.

Good, than it's a misunderstanding, we probably can have these autogenerated files as is, I don't have any strong preferences on this issue. I'll review the PR soon, it's really good that you've implemented RPC errors.

[Jim8y]

@shargon looks like we need another pr to focus on the coding style.

[cschuchardt88]

@Liaojinghui

@AnnaShaleva When we switch to dotnet7.0, IDE generate new files directly with the new coding style. I did not mean to change or update anything.

[Jim8y]

@Liaojinghui

LOL, @cschuchardt88 , this is not necessary. The core of my concern is "whether should we support new language style", not just the namespace. As is suggested by @AnnaShaleva , we can have another pr that update all existing code to the new style and fix other coding issues.

[Jim8y]

Hi @AnnaShaleva , I have updated the error code, please check. @shargon, please don't comment out the unused error code, its part of the protocol https://github.com/neo-project/proposals/pull/156.

[Jim8y]

@Liaojinghui, good, but some of earlier comments are not yet fixed. Could you give me the rights to resolve/unresolve conversations?

@AnnaShaleva i dont have the right to manage permissions.

[Jim8y]

@AnnaShaleva Now I think I have addressed all your comments

[AnnaShaleva]

@AnnaShaleva Now I think I have addressed all your comments

Let me check it one more time.

[shargon]

@AnnaShaleva Now I think I have addressed all your comments

Let me check it one more time.

ping @AnnaShaleva

[AnnaShaleva]

Sorry for the delayed review, I was really busy with the Bane project, I'll review this PR within a day.

[Jim8y]

All addressed. Expected to fail the UT. Need to wait https://github.com/neo-project/neo/pull/3076

[AnnaShaleva]

@Jim8y, could you also update the https://github.com/neo-project/neo-modules/pull/845#discussion_r1399517274?

[Jim8y]

@superboyiii good to go for testing.

[Jim8y]

We will need this PR in the mono repo, we don't have nuget, so, you can't update neo (for AlreadyInPool).

Can use local reference to test. Postpone the monorepo merge can make things unnecessarily complex. Waiting for a release is nonsense.

[roman-khimov]

We need this.