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

IR: Fix incorrect way of IR listing with notary Sidechain #2283

Merged
merged 1 commit into from
Mar 16, 2023

Conversation

cthulhu-rider
Copy link
Contributor

@cthulhu-rider cthulhu-rider commented Mar 16, 2023

Caught while testing nspcc-dev/neofs-contract#324 with removed innerRingList method.

Implementation did not comply with the requirements https://pkg.go.dev/github.com/nspcc-dev/neofs-contract@v0.16.0/netmap#InnerRingList

@cthulhu-rider cthulhu-rider added bug Something isn't working neofs-ir Inner Ring node application issues labels Mar 16, 2023
@codecov
Copy link

codecov bot commented Mar 16, 2023

Codecov Report

Merging #2283 (6e8a9fb) into master (e370aa9) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 6e8a9fb differs from pull request most recent head 0ffaf32. Consider uploading reports for the commit 0ffaf32 to get more accurate results

@@            Coverage Diff             @@
##           master    #2283      +/-   ##
==========================================
- Coverage   30.67%   30.66%   -0.01%     
==========================================
  Files         383      383              
  Lines       28314    28316       +2     
==========================================
- Hits         8685     8684       -1     
- Misses      18899    18902       +3     
  Partials      730      730              
Impacted Files Coverage Δ
...neofs-adm/internal/modules/morph/initialize_nns.go 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

After 3400a07 Inner Ring app switched
to calling `innerRingList` method of the Netmap contract to list Inner
Ring members in notary setting. Described approach is incorrect: in
notary installation Inner Ring composition should be fetched using Neo
native RoleManagement contract. Misbehavior was caused by basing the
condition on the notary setup of the Mainchain instead of Sidechain.

Use `IrFetcherWithNotary` as `IRFetcher` when Notary service is enabled
in the Sidechain regardless of the Mainchain setting.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Broken since 3400a07, wow.

@roman-khimov roman-khimov added this to the v0.36.0 milestone Mar 16, 2023
@roman-khimov roman-khimov merged commit 6699c04 into nspcc-dev:master Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working neofs-ir Inner Ring node application issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants