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

[Access] Refactor converters into separate files #4522

Merged
merged 8 commits into from
Jul 4, 2023

Conversation

peterargue
Copy link
Contributor

@peterargue peterargue commented Jun 27, 2023

The grpc converter methods were previously in one large 1200 line file, making it difficult to work with. This PR splits them up into different files per type and adds unittests validating that converting to protobuf and back results in the same object.

@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2023

Codecov Report

Merging #4522 (9dbabf7) into master (12f898b) will increase coverage by 0.34%.
The diff coverage is 59.10%.

@@            Coverage Diff             @@
##           master    #4522      +/-   ##
==========================================
+ Coverage   54.09%   54.44%   +0.34%     
==========================================
  Files         563      908     +345     
  Lines       56220    84896   +28676     
==========================================
+ Hits        30414    46218   +15804     
- Misses      23459    35114   +11655     
- Partials     2347     3564    +1217     
Flag Coverage Δ
unittests 54.44% <59.10%> (+0.34%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
admin/commands/storage/read_range_blocks.go 0.00% <0.00%> (ø)
...dmin/commands/storage/read_range_cluster_blocks.go 0.00% <0.00%> (ø)
cmd/execution_builder.go 0.00% <0.00%> (ø)
cmd/execution_config.go 0.00% <0.00%> (ø)
cmd/node_builder.go 93.87% <ø> (-2.07%) ⬇️
cmd/scaffold.go 14.76% <0.00%> (+0.26%) ⬆️
cmd/util/ledger/reporters/export_reporter.go 0.00% <ø> (ø)
cmd/utils.go 20.58% <ø> (+3.51%) ⬆️
cmd/verification_builder.go 0.00% <0.00%> (ø)
consensus/aggregators.go 0.00% <0.00%> (ø)
... and 85 more

... and 332 files with indirect coverage changes

Comment on lines 150 to 152
if len(p) == 0 {
return p, nil
}
Copy link
Contributor Author

@peterargue peterargue Jun 28, 2023

Choose a reason for hiding this comment

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

this is the only change to the converter logic. This was needed otherwise ccf will return an eof error if the payload is empty

Copy link
Member

@zhangchiqing zhangchiqing Jun 28, 2023

Choose a reason for hiding this comment

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

I wonder if this change/check is needed.

Is a valid CcfPayload always not empty? In other words, an empty payloads CcfPayload must be invalid? In that case, then the default behavior of returning an error (EOF error) is expected, the caller is better to receive an error instead of an empty bytes. Right?

cc @fxamacker

Copy link
Member

Choose a reason for hiding this comment

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

Is a valid CcfPayload always not empty?

Yes, valid CCF message is never empty.

the caller is better to receive an error instead of an empty bytes. Right?

Yes, I agree we should return error on empty data. JSON-CDC decoder currently returns error on empty data as well.

@peterargue peterargue force-pushed the petera/refactor-grpc-converters branch from 371b35f to 6191498 Compare June 28, 2023 22:22
Copy link
Member

@zhangchiqing zhangchiqing left a comment

Choose a reason for hiding this comment

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

Thanks for the refactor

@peterargue
Copy link
Contributor Author

bors merge

bors bot added a commit that referenced this pull request Jun 30, 2023
4522: [Access] Refactor converters into separate files r=peterargue a=peterargue

The grpc converter methods were previously in one large 1200 line file, making it difficult to work with. This PR splits them up into different files per type and adds unittests validating that converting to protobuf and back results in the same object.

Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Jun 30, 2023

Build failed:

@peterargue
Copy link
Contributor Author

bors retry

bors bot added a commit that referenced this pull request Jun 30, 2023
4522: [Access] Refactor converters into separate files r=peterargue a=peterargue

The grpc converter methods were previously in one large 1200 line file, making it difficult to work with. This PR splits them up into different files per type and adds unittests validating that converting to protobuf and back results in the same object.

Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Jun 30, 2023

Build failed:

@peterargue
Copy link
Contributor Author

bors merge

bors bot added a commit that referenced this pull request Jul 4, 2023
4522: [Access] Refactor converters into separate files r=peterargue a=peterargue

The grpc converter methods were previously in one large 1200 line file, making it difficult to work with. This PR splits them up into different files per type and adds unittests validating that converting to protobuf and back results in the same object.

Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Jul 4, 2023

Build failed:

@peterargue
Copy link
Contributor Author

bors merge

@bors bors bot merged commit b982ff1 into master Jul 4, 2023
@bors bors bot deleted the petera/refactor-grpc-converters branch July 4, 2023 22:08
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.

5 participants