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

state: drop NativeContract, fix #3430 #3439

Merged
merged 1 commit into from
May 17, 2024
Merged

Conversation

roman-khimov
Copy link
Member

@roman-khimov roman-khimov commented May 15, 2024

This is somewhat deeper than its C# counterpart, but the essence is the same, we return proper []state.Contract from getnativecontracts RPC and thus many of older types are no longer needed. Some hacks around UpdateCounter are removed as well.

The change can potentially affect third-party apps, but should be easy to adopt.

UPD: simplified.

Copy link

codecov bot commented May 15, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 86.15%. Comparing base (13020cc) to head (b1bb12d).
Report is 6 commits behind head on master.

Files Patch % Lines
pkg/core/blockchain.go 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3439      +/-   ##
==========================================
+ Coverage   86.14%   86.15%   +0.01%     
==========================================
  Files         331      331              
  Lines       38432    38441       +9     
==========================================
+ Hits        33106    33120      +14     
+ Misses       3800     3797       -3     
+ Partials     1526     1524       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

To be honest, I'm against of ContractBase removal. NativeContract can be removed, but not the ContractBase. ContractBase is a separate type which should not include update counter. It's used by HFSpecificContractMD which should not be aware of the contract's update counter, and update counter is a kind of Management-specific and dao-specific thing that I'd like to keep inside Management.

pkg/core/interop/context.go Outdated Show resolved Hide resolved
pkg/core/native/native_test/management_test.go Outdated Show resolved Hide resolved
@roman-khimov
Copy link
Member Author

ContractBase removal

But the only reason we have it that there was this separation of native/non-native contract state. e1d2a5b.

@AnnaShaleva
Copy link
Member

the only reason we have it that there was this separation

But right now it's not the only reason, because ContractBase is used by HFSpecificContractMD, and this entity is update-counter-agnostic.

@roman-khimov
Copy link
Member Author

ContractBase is used by HFSpecificContractMD, and this entity is update-counter-agnostic.

I still think we have a lot to improve in HF handling. But OK, I'll simplify this to fix getnativecontracts only.

@AnnaShaleva
Copy link
Member

I'm not completely against of ContractBase removal, but this code just doesn't work correctly.

This unifies getnativecontracts RPC API with regular contracts.

Signed-off-by: Roman Khimov <roman@nspcc.ru>
@roman-khimov roman-khimov changed the title state: drop NativeContract and ContractBase, fix #3430 state: drop NativeContract, fix #3430 May 16, 2024
@AnnaShaleva AnnaShaleva merged commit 647b8c7 into master May 17, 2024
19 of 21 checks passed
@AnnaShaleva AnnaShaleva deleted the getnativecontracts branch May 17, 2024 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants