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

Invalid MA is ignored #881

Merged
merged 2 commits into from
Apr 14, 2023
Merged

Invalid MA is ignored #881

merged 2 commits into from
Apr 14, 2023

Conversation

diegomrsantos
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Merging #881 (d18140a) into unstable (edbd35b) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           unstable     #881      +/-   ##
============================================
+ Coverage     84.21%   84.26%   +0.04%     
============================================
  Files            87       87              
  Lines         15125    15134       +9     
============================================
+ Hits          12738    12753      +15     
+ Misses         2387     2381       -6     
Impacted Files Coverage Δ
libp2p/multiaddress.nim 86.70% <100.00%> (+0.30%) ⬆️

... and 3 files with indirect coverage changes

@@ -1128,6 +1131,5 @@ proc getRepeatedField*(pb: ProtoBuffer, field: int,
if ma.isOk():
value.add(ma.get())
else:
value.setLen(0)
return err(ProtoError.IncorrectBlob)
debug "Not supported MultiAddress in blob", ma = item
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seems like the best solution
Maybe create a tryGetRepeatedField, that returns the number of failed parses? This way the caller can handle this however it wants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would any part of the current codebase use it and in which way? I believe it is better to add code only when it is required to solve a present problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the current version is not relevant anywhere, then we should change this proc to return the number of failed parses (or eq)

Copy link
Contributor

Choose a reason for hiding this comment

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

And update every usage accordingly (and check it's not used by nimbus & co)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed changes

@diegomrsantos diegomrsantos force-pushed the ma-pb-repeated-field branch 6 times, most recently from c23698e to 7c39793 Compare April 13, 2023 16:08
@diegomrsantos diegomrsantos enabled auto-merge (squash) April 14, 2023 12:04
@diegomrsantos diegomrsantos merged commit 0221aff into unstable Apr 14, 2023
@diegomrsantos diegomrsantos deleted the ma-pb-repeated-field branch April 14, 2023 12:05
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.

2 participants