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

api: Add stable marshaling #597

Merged
merged 7 commits into from
Jul 16, 2024
Merged

api: Add stable marshaling #597

merged 7 commits into from
Jul 16, 2024

Conversation

cthulhu-rider
Copy link
Contributor

@cthulhu-rider cthulhu-rider commented Jul 9, 2024

source code is ready and ready for review. Some tests left

@cthulhu-rider cthulhu-rider force-pushed the feature/stable-marshaling branch 2 times, most recently from 2185947 to 15a614f Compare July 9, 2024 19:58
Copy link

codecov bot commented Jul 9, 2024

Codecov Report

Attention: Patch coverage is 84.96481% with 235 lines in your changes missing coverage. Please review.

Project coverage is 53.27%. Comparing base (4a5fd58) to head (d1a668c).
Report is 3 commits behind head on master.

Files Patch % Lines
proto/internal/test/rand.go 0.00% 185 Missing ⚠️
proto/internal/test/encoding.go 29.23% 41 Missing and 5 partials ⚠️
proto/session/encoding.go 97.87% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #597       +/-   ##
===========================================
+ Coverage   39.56%   53.27%   +13.70%     
===========================================
  Files         147      164       +17     
  Lines       17496    19059     +1563     
===========================================
+ Hits         6922    10153     +3231     
+ Misses      10198     8489     -1709     
- Partials      376      417       +41     

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

@cthulhu-rider cthulhu-rider force-pushed the feature/stable-marshaling branch 5 times, most recently from 144b8e7 to 7bd816d Compare July 10, 2024 20:30
@cthulhu-rider cthulhu-rider marked this pull request as ready for review July 11, 2024 08:29

// All supported status codes.
const (
OK = 0
Copy link
Member

Choose a reason for hiding this comment

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

have you thought about some helper bit operations here? just to describe why it is 1024, 1025, 1026, 1027 and then 2048

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not needed, code is just a code, nobody cares what number is it

fieldDecimalPrecision
)

// MarshaledSize returns size of the Decimal in Protocol Buffers V3 format in
Copy link
Member

Choose a reason for hiding this comment

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

is it handmade? there was some concept with code generation, have you considered it? tree things were made with it: https://github.com/nspcc-dev/neofs-node/blob/22259ce755199d6e87331a3b58cc14449a36f69e/pkg/services/tree/types_neofs.pb.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have you considered it?

not yet, we need a proper testing 1st (it's coming with this PR). Then we can experiment with codegen

Copy link
Member

Choose a reason for hiding this comment

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

imo, a generator should have been written first, that is an idea about it. but it is late

They are not comparable. Previously, unit tests could fail.

Signed-off-by: Leonard Lyubich <[email protected]>
As they state in the docs.

Signed-off-by: Leonard Lyubich <[email protected]>
Previously, `MarshalToBytes` and `MarshalToRepeatedBytes` may have
encoded data incorrectly. If there was nothing left in the buffer to
accommodate the entire array, only the part that could fit was written.

Now the functions check that the buffer will actually hold the required
bytes, and only then calls built-in `copy`. Corresponding unit tests now
pass.

Signed-off-by: Leonard Lyubich <[email protected]>
Protocol Buffers V3 has no constants, and they are missing in the
generated code as well. To make them easier to access, they are exported
manually.

Signed-off-by: Leonard Lyubich <[email protected]>
Specific encoding of NeoFS messages is required to implement protocol
checksums and signatures.

Continues 51fa18f.

Signed-off-by: Leonard Lyubich <[email protected]>
This fixes
```
$ golangci-lint --version
golangci-lint has version 1.59.1 built with go1.22.3 from 1a55854a on 2024-06-09T18:08:33Z
$ golangci-lint run ./...
WARN [config_reader] The configuration option `output.format` is deprecated, please use `output.formats`
```

The configuration file is updated according to
https://golangci-lint.run/usage/configuration/.

Signed-off-by: Leonard Lyubich <[email protected]>
According to https://golangci-lint.run/usage/linters/#govet,
`check-shadowing` no longer exists. Now it should be
```yaml
govet:
  disable: [shadow]
```
but `shadow` is disabled by default. Thus, whole `govet` section is not
needed anymore.

```
$ golangci-lint --version
golangci-lint has version 1.59.1 built with go1.22.3 from 1a55854a on 2024-06-09T18:08:33Z
```

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider cthulhu-rider marked this pull request as ready for review July 16, 2024 08:47
@carpawell carpawell merged commit fa3ad10 into master Jul 16, 2024
12 checks passed
@carpawell carpawell deleted the feature/stable-marshaling branch July 16, 2024 16:31
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