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

smartcontract: update nns wrapper #3291

Merged
merged 1 commit into from
Jan 27, 2024
Merged

smartcontract: update nns wrapper #3291

merged 1 commit into from
Jan 27, 2024

Conversation

AliceInHunterland
Copy link
Contributor

@AliceInHunterland AliceInHunterland commented Jan 16, 2024

Added missing function from nns smart-contract manifest.

Closes #2732.

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Attention: 140 lines in your changes are missing coverage. Please review.

Comparison is base (36b8921) 85.21% compared to head (000109b) 84.97%.

Files Patch % Lines
pkg/rpcclient/nns/contract.go 38.59% 138 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3291      +/-   ##
==========================================
- Coverage   85.21%   84.97%   -0.24%     
==========================================
  Files         327      328       +1     
  Lines       44457    44707     +250     
==========================================
+ Hits        37882    37992     +110     
- Misses       5070     5208     +138     
- Partials     1505     1507       +2     

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

pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract_test.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract_test.go Outdated Show resolved Hide resolved
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.

Add reference to initial issue to commit/PR description.

pkg/rpcclient/nns/contract.go Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Show resolved Hide resolved
pkg/rpcclient/nns/contract_test.go Show resolved Hide resolved
@AnnaShaleva
Copy link
Member

Linter is failing due to underscores in func names, so let's resolve this issue in #3296, and for now let's have underscore-free names.

pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract_test.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract_test.go Show resolved Hide resolved
pkg/rpcclient/nns/contract_test.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Show resolved Hide resolved
pkg/rpcclient/nns/iterators.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract_test.go Outdated Show resolved Hide resolved
@AnnaShaleva
Copy link
Member

Need to be rebased onto master.

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.

And we need to think about the destination place for NNS wrapper because it's not native contract anymore.

pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract.go Outdated Show resolved Hide resolved
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.

s/Refs #2732/Close #2732. in the commit message, because this single commit closes the whole issue. Also, in the same commit we need to move this wrapper to another directory.

@roman-khimov, I've consider the pkg/rpcwrappers destination directory for this package, because currently we have rpcclient upper-level directory with rpc prefix, so it looks similar. I also considered something like pkg/nonnative-wrappers if one day we're going to have simple wrappers for contracts (but it's only applicable for go contracts, so hardly ever we'll add something). Maybe you have something in mind? Or maybe @AliceInHunterland?

pkg/rpcclient/nns/contract.go Show resolved Hide resolved
pkg/rpcclient/nns/contract_test.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract_test.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract_test.go Outdated Show resolved Hide resolved
pkg/rpcclient/nns/contract_test.go Show resolved Hide resolved
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.

A batch of setters left uncovered, but otherwise LGTM.

@roman-khimov, need your opinion on wrapper location path.

@roman-khimov
Copy link
Member

rpcwrappers seem to be redundant here. Ideally, external contracts would have wrappers in their respective repositories (like neofs-contract), in this case that'd be https://github.com/neo-project/non-native-contracts. This is perfectly fine for most of contracts. It can be a separate repo as well if it's not accepted into the upstream one and that's also OK in most cases. The problem is that we're going to use this wrapper in our own CLI, so suddenly we have a circular dependency. Which is not the end of the world (hi, nspcc-dev/dbft#2), but still an inconvenience, especially if we're considering updates.

Many other rpcclient subpackages are wrappers in their essence as well.

@AnnaShaleva
Copy link
Member

external contracts would have wrappers in their respective repositories

Not the NNS case, hardly ever we'll merge these changes in the https://github.com/neo-project/non-native-contracts.

It can be a separate repo as well

I saw this comment in the original issue (#2732 (comment)), but I don't want to introduce this circular dependency, because it's irritating to update it. I don't want to add more updating problems, we already have interop deps updating scripts, it's enough I think.

Many other rpcclient subpackages are wrappers in their essence as well.

Do you suggest to keep it as is in the pkg/rpcclient/nns?

@roman-khimov
Copy link
Member

Yep, pkg/rpclient/nns seems to be OK given all the constraints, rpcwrappers just don't improve much.

@AnnaShaleva
Copy link
Member

@AliceInHunterland, please, move the wrapper back to rpcclient/nns.

Added missing function from smart-contract manifest.

Close #2732

Signed-off-by: Ekaterina Pavlova <ekt@morphbits.io>
@roman-khimov roman-khimov merged commit 9d95a26 into master Jan 27, 2024
16 of 19 checks passed
@roman-khimov roman-khimov deleted the nns_wrapper branch January 27, 2024 08:25
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.

NNS wrapper update
3 participants