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

Update neofs-contract to v0.20.0 #2872

Merged
merged 2 commits into from
Jul 30, 2024
Merged

Conversation

roman-khimov
Copy link
Member

As a part of nspcc-dev/neofs-contract#384 I'd like to unify different code dealing with them.

Copy link

codecov bot commented Jun 14, 2024

Codecov Report

Attention: Patch coverage is 24.24242% with 25 lines in your changes missing coverage. Please review.

Project coverage is 23.65%. Comparing base (2a70adb) to head (ed0048e).

Files Patch % Lines
...fs-adm/internal/modules/morph/initialize_deploy.go 0.00% 12 Missing ⚠️
pkg/morph/event/netmap/update_peer.go 0.00% 4 Missing ⚠️
pkg/morph/client/netmap/netmap.go 72.72% 2 Missing and 1 partial ⚠️
pkg/morph/client/netmap/update_state.go 0.00% 2 Missing ⚠️
...md/neofs-adm/internal/modules/morph/remove_node.go 0.00% 1 Missing ⚠️
cmd/neofs-node/container.go 0.00% 1 Missing ⚠️
pkg/innerring/contracts.go 0.00% 1 Missing ⚠️
pkg/morph/client/container/get.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2872      +/-   ##
==========================================
- Coverage   23.73%   23.65%   -0.09%     
==========================================
  Files         775      774       -1     
  Lines       44933    44872      -61     
==========================================
- Hits        10666    10613      -53     
+ Misses      33416    33415       -1     
+ Partials      851      844       -7     

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

@roman-khimov roman-khimov force-pushed the adm-new-contract-archive branch 3 times, most recently from 657c5c2 to 9930ecd Compare June 14, 2024 10:07
@roman-khimov
Copy link
Member Author

@evgeniiz321, tests fail here and I can't get any logs.

@roman-khimov
Copy link
Member Author

This updates contracts as well, so it's gonna be postponed till we have a proper 0.20.0.

@roman-khimov roman-khimov force-pushed the adm-new-contract-archive branch 2 times, most recently from bd98176 to 9d1ed35 Compare June 14, 2024 15:49
@roman-khimov roman-khimov mentioned this pull request Jul 26, 2024
@roman-khimov roman-khimov marked this pull request as ready for review July 30, 2024 10:33
@roman-khimov roman-khimov added this to the v0.43.0 milestone Jul 30, 2024
@roman-khimov roman-khimov changed the title adm: add support for the new neofs-contract archives Update neofs-contract to v0.20.0 Jul 30, 2024
Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

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

Tests adaption.

@@ -372,11 +372,17 @@ func (c *initializeContext) readContracts(names []string) error {
func readContract(ctrPath, ctrName string) (*contractState, error) {
rawNef, err := os.ReadFile(filepath.Join(ctrPath, ctrName+"_contract.nef"))
if err != nil {
return nil, fmt.Errorf("can't read NEF file for %s contract: %w", ctrName, err)
rawNef, err = os.ReadFile(filepath.Join(ctrPath, "contract.nef"))
Copy link
Member

@carpawell carpawell Jul 30, 2024

Choose a reason for hiding this comment

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

go this way only if file not found? also, do you plan to drop it in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

@carpawell is faster, but i'll leave my comment anyway

err may be other than 404, so i'd either:

  1. try other file on 404 only
  2. try anyway, but keep both errors when it fails too

same for manifest files

Copy link
Member Author

Choose a reason for hiding this comment

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

I doubt it's worth the trouble, but fixed anyway.

Copy link
Member

Choose a reason for hiding this comment

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

also, do you plan to drop it in the future?

why do we try to support both ways? how long?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it still is a generic tool. We'll see. An issue can be created.

Copy link
Contributor

@cthulhu-rider cthulhu-rider left a comment

Choose a reason for hiding this comment

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

good overall

cmd/neofs-adm/internal/modules/morph/initialize_deploy.go Outdated Show resolved Hide resolved
@@ -4,7 +4,7 @@ import (
"fmt"

"github.com/nspcc-dev/neo-go/pkg/vm/stackitem"
netmapcontract "github.com/nspcc-dev/neofs-contract/contracts/netmap"
netmaprpc "github.com/nspcc-dev/neofs-contract/rpc/netmap"
Copy link
Contributor

Choose a reason for hiding this comment

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

i liked previous alias more tbh

As a part of nspcc-dev/neofs-contract#384 I'd like
to unify different code dealing with them.

Signed-off-by: Roman Khimov <[email protected]>
Now updating neofs-contract dependency will automatically update contracts,
see nspcc-dev/neofs-contract#384

This also updates contracts dependency to 0.20.0 adjusting the code where
neccessary.

Signed-off-by: Roman Khimov <[email protected]>
@cthulhu-rider cthulhu-rider merged commit 1ebe897 into master Jul 30, 2024
19 of 22 checks passed
@cthulhu-rider cthulhu-rider deleted the adm-new-contract-archive branch July 30, 2024 13:09
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.

3 participants