*: Replace `github.com/nspcc-dev/neofs-api-go/v2` module #667

cthulhu-rider · 2024-12-11T10:55:47Z

ReadFromV2 / WriteToV2
request/response sign/verify funcs
Client

cthulhu-rider · 2024-12-11T10:57:57Z

accounting/decimal.go

 )

 // Decimal represents decimal number for accounting operations.
 //
-// Decimal is mutually compatible with github.com/nspcc-dev/neofs-api-go/v2/accounting.Decimal
-// message. See ReadFromV2 / WriteToV2 methods.
+// Decimal is mutually compatible with [protoaccounting.Decimal] message. See


suggest to drop such statements since regular user is not interested in this

@roman-khimov @carpawell

cthulhu-rider · 2024-12-11T10:58:28Z

accounting/decimal.go

-// See also WriteToV2.
-func (d *Decimal) ReadFromV2(m accounting.Decimal) error {
-	*d = Decimal(m)
+// See also [Decimal.ProtoMessage].


and this, Client and node developers know what to use

cthulhu-rider · 2024-12-11T11:06:40Z

accounting/decimal.go

 //
-// See also WriteToV2.
-func (d *Decimal) ReadFromV2(m accounting.Decimal) error {


note that temporary Deprecated mark wont work: conflicting import panic occurs

Signed-off-by: Leonard Lyubich <[email protected]>

cthulhu-rider · 2024-12-16T18:30:18Z

coding is now complete. Looking at the failed AIO tests

im almost 100% sure some diffs can be considered as a separate change not related to the main commit. Due to the large volume, I decided not to bother and get a fully working final result. Separation would require more time than benefit

also pay attention to doc proposals (@roman-khimov):

cthulhu-rider · 2024-12-16T18:30:34Z

missclick

While an unspecified (nil) embedded field of `message` protobuf type is encoded as nothing, the zero is still reflected in the message as an encoded field number plus zero. This fixes the test and makes correct assertion of zero message encoding. The tests fails meaning there is a bug for now. The test is also made independent with the nil one to make the not fail each other. Signed-off-by: Leonard Lyubich <[email protected]>

Fixes the problem detected in d44ef3b via `reflect` package as well as `neofs-api-go` module does. Another way would be to check each field in the implementation of `MarshalStable([]byte)` interface, but this would add a significant amount of code. Benchmark showed almost no performance loss: ``` goos: linux goarch: amd64 pkg: github.com/nspcc-dev/neofs-sdk-go/internal/proto cpu: Intel(R) Core(TM) i5-10210U CPU @ 1.60GHz │ old.txt │ new.txt │ │ sec/op │ sec/op vs base │ MarshalEmbedded-8 26.00n ± 31% 27.59n ± 2% ~ (p=0.393 n=10) │ old.txt │ new.txt │ │ B/op │ B/op vs base │ MarshalEmbedded-8 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹ ¹ all samples are equal │ old.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ MarshalEmbedded-8 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹ ¹ all samples are equal ``` Signed-off-by: Leonard Lyubich <[email protected]>

They will be useful for binary and JSON encoding implementations. Signed-off-by: Leonard Lyubich <[email protected]>

Leverage power of generics to reduce amount of code and simplify support. Signed-off-by: Leonard Lyubich <[email protected]>

They should be encoded similarly to zero messages. Signed-off-by: Leonard Lyubich <[email protected]>

codecov · 2024-12-17T16:09:56Z

Codecov Report

Attention: Patch coverage is 91.33384% with 230 lines in your changes missing coverage. Please review.

Project coverage is 60.45%. Comparing base (0b9bb74) to head (fade159).
Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/proto/encoding.go	7.93%	58 Missing ⚠️
netmap/policy.go	87.25%	26 Missing and 6 partials ⚠️
object/object.go	92.98%	14 Missing and 5 partials ⚠️
object/attribute.go	52.77%	17 Missing ⚠️
client/object_get.go	90.71%	7 Missing and 6 partials ⚠️
object/link.go	75.67%	6 Missing and 3 partials ⚠️
client/status/common.go	85.18%	7 Missing and 1 partial ⚠️
crypto/ecdsa/wallet_connect.go	77.77%	4 Missing and 2 partials ⚠️
crypto/proto.go	96.89%	4 Missing and 2 partials ⚠️
object/lock.go	77.77%	4 Missing and 2 partials ⚠️
... and 17 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #667      +/-   ##
==========================================
- Coverage   60.94%   60.45%   -0.50%     
==========================================
  Files         165      165              
  Lines       22817    22238     -579     
==========================================
- Hits        13906    13444     -462     
+ Misses       8544     8449      -95     
+ Partials      367      345      -22

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

Previously, implementations of `MarshaledSize` and `MarshalStable` interfaces ignored nil elements in repeated message fields, resulting in the following behavior: if _m_ of _n_ fields are nil, only _n-m_ ones were encoded and further restored. Although the "nilness" of the element makes little sense since it is not reflected in Protocol Buffers binary format, this behavior was definitely unexpected, i.e. a bug. This brings the solution: encode nils similarly to zeros. This is pretty legit since the absence of a field is interpreted as zero in protobuf. At the same time, the number of array elements will be preserved, which is extremely important. An additional motivation was to compare the behavior with `github.com/nspcc-dev/neofs-api-go/v2` module, which behaves in the same way. Signed-off-by: Leonard Lyubich <[email protected]>

roman-khimov

Can we go type by type with this?

accounting/decimal.go

cthulhu-rider · 2024-12-19T16:29:51Z

Can we go type by type with this?

all types follow the same approach. We can discuss on one example yeah

roman-khimov · 2024-12-19T18:37:24Z

I mean the PR. Initial patches can be merged as is. But types better be converted one after another, at least per-package. Some other client changes also deserve additional attention, but it's impossible to check.

cthulhu-rider · 2024-12-19T18:52:54Z

got it. No, I won't do this. All these types follow the same approach. Maintaining 20 commits 1 depending from all others is just a meaningless overhead. Pkgs can be reviewed one by one instead. Although I understand ur concern, it'd be hard to review for me as well

also, I wouldn't be able to fix panics in tests

carpawell

I tried my best, my browser almost died. Agree that the last commit may be tried to be split on api-go and functional/refactor changes but currently it will be hard to do so not worth it.
Do we expect any proto API v3? There is no "v2" suffixes anymore but previously we thought that it was important.

internal/proto/encoding.go

accounting/decimal.go

client/accounting.go

crypto/proto.go

eacl/table.go

session/container.go

object/object_copy.go

cthulhu-rider · 2024-12-25T13:03:12Z

i'll try my best to split 1 commit into 3 ones:
1 all packages except client
2. client
3. simplify client implementation

cthulhu-rider · 2024-12-25T14:33:19Z

nah, ive done my maximum and just split client/* changes from the rest. No matter how hard I try to go pkg-by-pkg, it only leads to panics, redundant hacks and more extra coding. At this point it's just not worth the time

netmap/policy.go

roman-khimov · 2024-12-26T10:11:41Z

object/link.go

@@ -36,100 +38,97 @@ func (o Object) ReadLink(l *Link) error {
 //
 // See also [Link.Unmarshal].
 func (l *Link) Marshal() []byte {
-	return (*v2object.Link)(l).StableMarshal(nil)
+	if l == nil || len(l.children) == 0 {
+		return nil


nil? I'd expect []byte{}.

how is it better?

I think it's more correct, I don't expect Marshal() to ever return nil. If we have no data in the end --- that's []byte{}, successful encoding as an empty byte slice. nil just adds ambiguity for the consumer of this API, has it failed, is this the correct result? Also that's the way old encoder worked.

failure is error. Here is no error, and can never be. If switching b/w empty slice and nil breaks nothing (as it is), and docs tell nothing about their diff, nil is completely ok to me

has it failed, is this the correct result?

either correct or panic, nothing else possible

roman-khimov · 2024-12-26T10:13:09Z

object/lock.go

 }

 // Marshal encodes the [Lock] into a NeoFS protocol binary format.
 //
 // See also [Lock.Unmarshal].
 func (x Lock) Marshal() []byte {
-	return (*v2object.Lock)(&x).StableMarshal(nil)
+	if len(x.members) == 0 {
+		return nil


object/object.go

crypto/proto.go

Since 527c964, proto-generated code is located in the current code library. Previously, the neofs-api-go module's code was used for protocol interactions. In essence, it is a super-layer on top of the `google.golang.org/protobuf` module with only one feature: stable marshalling (Protocol Buffers with ascending field order). Since it is now provided by local `proto/*` packages, there is no longer a need for a separate module. In addition to trimming the code base and refactoring, a bonus is the allocation of fewer intermediate Go objects, which will have a beneficial effect on runtime. Although most of the changes are refactorings, the following changes have been applied: 1. All exported elements that depended on neofs-api-go types are permanently removed. They could not be excluded via a temporary deprecation mark as this would cause a conflict with importing ''*.pb.go' files. Such elements were not recommended for use in advance, so most of the updated software will not suffer from breakages. 2. `neofs-api-go` a bug with interpreting enums as unsigned numbers, while protobuf lib decodes them into `~int32`. In addition, during encoding/decoding, all unknown enum values were set to zero, which could lead to data loss. This commit fixes both issues. Separating the fix from the refactoring would have required adding artificial buggy code, it was decided not to generate it. 3. Test binaries/JSONs/signatures that followed the erroneous behavior of p.2 are fixed. 4. While replacement of `ReadFromV2` methods called `FromProtoMessage`, `ProtoMessage` method replacing `WriteToV2`, taking into account the proposal from #532 and existing use cases, now dynamically allocates the message structure. NOTE: `client` package is going to be changed the same way in the following commits. NOTE 2: some tests panic due to protobuf namespace conflict, which will be fixed automatically after the move is complete. Closes #214. Refs #226. Refs #532. Fixes #606. Refs #652. Signed-off-by: Leonard Lyubich <[email protected]>

Continues 8a26264 and finalizes deprecation of `neofs-api-go/v2` module in favor of `proto/` packages. Request/response signing and verification functions have been added to `crypto` package A. They are used by the API client and will also be used by https://github.com/nspcc-dev/neofs-node. API client unit tests and AIO integration ones PASS, therefore, there is no more way to track more regressions for now. If problems arise in apps when updating the lib, fixes will be added later before the major release. Refs #270. Closes #452. Refs #532. Closes #551 (it's almost impossible to do now). Signed-off-by: Leonard Lyubich <[email protected]>

Signed-off-by: Leonard Lyubich <[email protected]>

carpawell · 2024-12-27T08:45:42Z

internal/proto/encoding.go

+		if !isMessageNil(v[i]) {
+			sz += SizeEmbedded(num, v[i])
+		} else {
+			sz += SizeEmbedded(num, M(&zero))


i do not understand, it is a really complicated generic func that just tries to handle zero value in a generic fashion? still do not understand why it is needed since #667 (comment) thread

for the sake of callers - they directly pass codegened fields for encoding. This allowed to drop for loop for all repeated fields. Btw, tbh, it this rly so complex func?

Btw, tbh, it this rly so complex func?

1 because of its definition

func SizeRepeatedMessages[T any, M interface { *T Message }](num protowire.Number, v []M) int {

2 because of magic T any generic type. it does not do what is usually done about generics like "i want certain types here", it does not take part in args

i tried my best to generalize []*T args and encode repeated fields in one line of code. And i agree that definition is not very convenient to read. May u pls suggest more readable definition?

P.S. since this is merged, share it in the new issue pls

cthulhu-rider commented Dec 11, 2024

View reviewed changes

cthulhu-rider force-pushed the avoid-apigo branch 5 times, most recently from fcf2c0b to ca23744 Compare December 13, 2024 16:35

accounting: Fix test assert

e131cda

Signed-off-by: Leonard Lyubich <[email protected]>

cthulhu-rider force-pushed the avoid-apigo branch 4 times, most recently from 6e77f53 to c0a8824 Compare December 16, 2024 18:21

cthulhu-rider closed this Dec 16, 2024

cthulhu-rider reopened this Dec 16, 2024

cthulhu-rider mentioned this pull request Dec 17, 2024

internal/proto: Fix encoding of zero embedded messages #672

Closed

cthulhu-rider added 5 commits December 17, 2024 18:49

proto: Add helper functions for Message encoding

54d1de6

They will be useful for binary and JSON encoding implementations. Signed-off-by: Leonard Lyubich <[email protected]>

internal/proto: Refactor sizing and encoding of embedded repeated fields

f34803d

Leverage power of generics to reduce amount of code and simplify support. Signed-off-by: Leonard Lyubich <[email protected]>

proto: Test encoding of nil elements in repeated message fields

57e10da

They should be encoded similarly to zero messages. Signed-off-by: Leonard Lyubich <[email protected]>

cthulhu-rider force-pushed the avoid-apigo branch from c0a8824 to e6b167e Compare December 17, 2024 16:02

cthulhu-rider force-pushed the avoid-apigo branch from e6b167e to 4cb71b4 Compare December 17, 2024 16:12

cthulhu-rider force-pushed the avoid-apigo branch 3 times, most recently from 8979416 to ed4d990 Compare December 17, 2024 18:30

cthulhu-rider requested a review from carpawell as a code owner December 18, 2024 13:42

roman-khimov reviewed Dec 19, 2024

View reviewed changes

accounting/decimal.go Show resolved Hide resolved

carpawell reviewed Dec 23, 2024

View reviewed changes

cthulhu-rider force-pushed the avoid-apigo branch from 08e09e1 to 82acb94 Compare December 25, 2024 11:50

cthulhu-rider marked this pull request as draft December 25, 2024 13:03

cthulhu-rider force-pushed the avoid-apigo branch 3 times, most recently from 155f3d4 to ffc93a1 Compare December 25, 2024 14:28

cthulhu-rider marked this pull request as ready for review December 25, 2024 14:34

cthulhu-rider requested review from carpawell and roman-khimov December 25, 2024 14:34

roman-khimov reviewed Dec 26, 2024

View reviewed changes

cthulhu-rider force-pushed the avoid-apigo branch from ffc93a1 to a48023c Compare December 26, 2024 15:20

cthulhu-rider mentioned this pull request Dec 26, 2024

Reuse available capacity of slices in CopyTo #674

Closed

cthulhu-rider force-pushed the avoid-apigo branch from a48023c to 1efaa3a Compare December 26, 2024 15:29

cthulhu-rider requested a review from roman-khimov December 26, 2024 15:37

cthulhu-rider added 3 commits December 26, 2024 19:17

netmap/policy: Add test checking CopyTo clones empty sub-filter slice

fade159

Signed-off-by: Leonard Lyubich <[email protected]>

cthulhu-rider force-pushed the avoid-apigo branch from 1efaa3a to fade159 Compare December 26, 2024 16:30

roman-khimov approved these changes Dec 26, 2024

View reviewed changes

carpawell reviewed Dec 27, 2024

View reviewed changes

carpawell approved these changes Dec 27, 2024

View reviewed changes

roman-khimov merged commit 5f693d2 into master Dec 27, 2024
10 checks passed

roman-khimov deleted the avoid-apigo branch December 27, 2024 09:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Replace `github.com/nspcc-dev/neofs-api-go/v2` module #667

*: Replace `github.com/nspcc-dev/neofs-api-go/v2` module #667

cthulhu-rider commented Dec 11, 2024 •

edited

Loading

cthulhu-rider Dec 11, 2024

cthulhu-rider Dec 11, 2024

cthulhu-rider Dec 11, 2024

cthulhu-rider commented Dec 16, 2024

cthulhu-rider commented Dec 16, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

roman-khimov left a comment

cthulhu-rider commented Dec 19, 2024

roman-khimov commented Dec 19, 2024

cthulhu-rider commented Dec 19, 2024

carpawell left a comment

cthulhu-rider commented Dec 25, 2024

cthulhu-rider commented Dec 25, 2024 •

edited

Loading

roman-khimov Dec 26, 2024

cthulhu-rider Dec 26, 2024

roman-khimov Dec 26, 2024

cthulhu-rider Dec 26, 2024 •

edited

Loading

roman-khimov Dec 26, 2024

carpawell Dec 27, 2024

cthulhu-rider Dec 27, 2024

carpawell Dec 27, 2024 •

edited

Loading

cthulhu-rider Dec 27, 2024 •

edited

Loading

*: Replace github.com/nspcc-dev/neofs-api-go/v2 module #667

*: Replace github.com/nspcc-dev/neofs-api-go/v2 module #667

Conversation

cthulhu-rider commented Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cthulhu-rider commented Dec 16, 2024

cthulhu-rider commented Dec 16, 2024 • edited Loading

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

roman-khimov left a comment

Choose a reason for hiding this comment

cthulhu-rider commented Dec 19, 2024

roman-khimov commented Dec 19, 2024

cthulhu-rider commented Dec 19, 2024

carpawell left a comment

Choose a reason for hiding this comment

cthulhu-rider commented Dec 25, 2024

cthulhu-rider commented Dec 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cthulhu-rider Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

cthulhu-rider Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

*: Replace `github.com/nspcc-dev/neofs-api-go/v2` module #667

*: Replace `github.com/nspcc-dev/neofs-api-go/v2` module #667

cthulhu-rider commented Dec 11, 2024 •

edited

Loading

cthulhu-rider commented Dec 16, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

cthulhu-rider commented Dec 25, 2024 •

edited

Loading

cthulhu-rider Dec 26, 2024 •

edited

Loading

carpawell Dec 27, 2024 •

edited

Loading

cthulhu-rider Dec 27, 2024 •

edited

Loading