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

multiaddr_decode: Add default port logic #235

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

colluca
Copy link
Contributor

@colluca colluca commented Sep 26, 2024

In the case of the multi-address decoder, it makes sense to forward a request to the default port if the request's address set is not fully contained in the union of all rules' address sets, i.e. if the intersection is non-empty.

To simplify the module, we expect this union to be externally defined and passed to the module as a parameter.

This PR implements the logic required to implement a default port for the multi-address decoder.

Copy link
Collaborator

@niwis niwis left a comment

Choose a reason for hiding this comment

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

Thank you Luca, LGTM in general. Could you elaborate on how the union of all rules should be computed? Maybe there would be a point in adding the corresponding logic in this module (perhaps as a function) to make this easier to reuse.

Since the PR introduces interface changes, I am tagging this as v2. Let me know if you need a release tag, then I can create a release candidate.

@niwis niwis added the v2 label Oct 4, 2024
@colluca
Copy link
Contributor Author

colluca commented Oct 4, 2024

@niwis thanks for your feedback.

It's not always easy to calculate the union, and I suspect the set of address sets representable with the (addr, mask) encoding is not even closed under the union operation.

Nonetheless, I can provide some functions to e.g. convert rules between (start, end) and (addr, mask) forms, perform unions and so on in the most common cases, perhaps guarded somehow with assertions.

Where would you suggest adding these?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants