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

DeprecatedIn for events #3362

Merged
merged 8 commits into from
Jul 5, 2024
Merged

DeprecatedIn for events #3362

merged 8 commits into from
Jul 5, 2024

Conversation

shargon
Copy link
Member

@shargon shargon commented Jun 27, 2024 •
edited
Loading

Description

Partially close #3210

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

How Has This Been Tested?

  • Current tests

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

@shargon shargon marked this pull request as ready for review June 27, 2024 13:50
@shargon
Copy link
Member Author

shargon commented Jun 27, 2024

Not related error with windows unit tests.

CSC : error CS2012: Cannot open 'D:\a\neo\neo\src\Plugins\RpcServer\obj\Debug\net8.0\RpcServer.dll' for writing -- 'The process cannot access the file 'D:\a\neo\neo\src\Plugins\RpcServer\obj\Debug\net8.0\RpcServer.dll' because it is being used by another process.' [D:\a\neo\neo\src\Plugins\RpcServer\RpcServer.csproj]

@shargon shargon changed the title DeprecatedIn for events DeprecatedIn for events Jun 27, 2024
@AnnaShaleva
Copy link
Member

@shargon, good, that is the last thing that is missing from #3210, thus I think you may adjust your PR message and say that this PR closes #3210. We have a separate issue for natives destroy (#3363).

Jim8y
Jim8y previously approved these changes Jun 27, 2024
View reviewed changes
@vncoelho
Copy link
Member

@shargon, good, that is the last thing that is missing from #3210, thus I think you may adjust your PR message and say that this PR closes #3210. We have a separate issue for natives destroy (#3363).

Very well

vncoelho
vncoelho reviewed Jun 27, 2024
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

Perhaps a minimum UT would be good, can we simulate that?

@Jim8y
Copy link
Contributor

Jim8y commented Jun 27, 2024

Perhaps a minimum UT would be good, can we simulate that?

UT on this would be complex, need a how new hardfork environment setting. This is like a copy of deprecatedIn for method, should be fine as its already working properly in 3.7.4.

AnnaShaleva
AnnaShaleva reviewed Jun 28, 2024
View reviewed changes
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.

LGTM.

src/Neo/SmartContract/Native/ContractEventAttribute.cs Show resolved Hide resolved
src/Neo/SmartContract/Native/ContractEventAttribute.cs Show resolved Hide resolved
vncoelho
vncoelho reviewed Jun 28, 2024
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

Does it cover all types of events and notifications, @shargon ? As @AnnaShaleva commented?

Perhaps some UTs are necessary.

@AnnaShaleva
Copy link
Member

Does it cover all types of events and notifications

It does.

Perhaps some UTs are necessary.

But the thing is that we just don't have a use-case for it for real native contracts, thus it will be hard to test. The code is rather simple, and we can test it once we get a native notification that should be disabled.

@vncoelho
Copy link
Member

Does it cover all types of events and notifications

It does.

Perhaps some UTs are necessary.

But the thing is that we just don't have a use-case for it for real native contracts, thus it will be hard to test. The code is rather simple, and we can test it once we get a native notification that should be disabled.

So maybe this PR should be in stand by until there as well.

@roman-khimov
Copy link
Contributor

It should be possible to test with some fake native contract, but I'm not sure the codebase allows for this (plugging an additional contract into the current set from test).

@shargon
Copy link
Member Author

shargon commented Jul 1, 2024

UT was added

cschuchardt88
cschuchardt88 reviewed Jul 1, 2024
View reviewed changes
src/Neo/SmartContract/Native/ContractEventAttribute.cs
{
DeprecatedIn = deprecatedIn;
}

Copy link
Member

Choose a reason for hiding this comment

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

Do you really need all these constructors? Unless their required and absolutely needed than yes, otherwise just make properties your setting Public. Public for optional.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes because enum doesn't work as arguments :(

src/Neo/SmartContract/Native/ContractMethodAttribute.cs Show resolved Hide resolved
AnnaShaleva
AnnaShaleva approved these changes Jul 1, 2024
View reviewed changes
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.

LGTM.

src/Neo/SmartContract/Native/ContractEventAttribute.cs Show resolved Hide resolved
src/Neo/SmartContract/Native/ContractMethodAttribute.cs Show resolved Hide resolved
tests/Neo.UnitTests/SmartContract/Native/UT_NativeContract.cs Show resolved Hide resolved
@shargon shargon mentioned this pull request Jul 4, 2024
Open
15 tasks
@shargon shargon added the blocker Issues that are blocking other issues. Check issues details to see what it is blocking. label Jul 4, 2024
@NGDAdmin NGDAdmin merged commit b7bc07b into master Jul 5, 2024
6 of 7 checks passed
@NGDAdmin NGDAdmin deleted the core-deprecate-events branch July 5, 2024 02:55
Jim8y added a commit to Jim8y/neo that referenced this pull request Jul 5, 2024
@Jim8y
Merge branch 'snapshot-tests' of github.com:Jim8y/neo into snapshot-t…
88301ba 
…ests

* 'snapshot-tests' of github.com:Jim8y/neo:
  Fix crash when comparing ContractPermissionDescriptor (neo-project#3396)
  `DeprecatedIn` for events (neo-project#3362)
  Fix download tips (neo-project#3395)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cschuchardt88 cschuchardt88 cschuchardt88 approved these changes

@Jim8y Jim8y Jim8y approved these changes

@AnnaShaleva AnnaShaleva AnnaShaleva approved these changes

@roman-khimov roman-khimov Awaiting requested review from roman-khimov

@NGDAdmin NGDAdmin Awaiting requested review from NGDAdmin

@vncoelho vncoelho Awaiting requested review from vncoelho

Assignees
No one assigned
Labels
blocker Issues that are blocking other issues. Check issues details to see what it is blocking. ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve native contract update mechanism
7 participants
@shargon @AnnaShaleva @vncoelho @Jim8y @roman-khimov @cschuchardt88 @NGDAdmin