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

docs: Swarm.AddrFilters ip address example fix #9386

Closed
wants to merge 1 commit into from
Closed

docs: Swarm.AddrFilters ip address example fix #9386

wants to merge 1 commit into from

Conversation

alx696
Copy link

@alx696 alx696 commented Nov 8, 2022

The current example 192.168.0.0/16 is wrong:

2022-11-08T08:47:41.660+0800	ERROR	core	core/builder.go:157	constructing the node: could not build arguments for function "github.com/ipfs/kubo/core/node".PeerWith.func1 (/home/m/go/pkg/mod/github.com/ipfs/[email protected]/core/node/peering.go:29): failed to build *peering.PeeringService: could not build arguments for function "github.com/ipfs/kubo/core/node".Peering (/home/m/go/pkg/mod/github.com/ipfs/[email protected]/core/node/peering.go:14): failed to build host.Host: could not build arguments for function "github.com/ipfs/kubo/core/node/libp2p".Host (/home/m/go/pkg/mod/github.com/ipfs/[email protected]/core/node/libp2p/host.go:40): could not build value group []config.Option[group="libp2p"]: received non-nil error from function "github.com/ipfs/kubo/core/node/libp2p".AddrFilters.func1 (/home/m/go/pkg/mod/github.com/ipfs/[email protected]/core/node/libp2p/addrs.go:14): incorrectly formatted address filter in config: 192.168.0.0/16

/ip4/192.168.0.0/ipcidr/16 is correct.

Swarm.AddrFilters example not right.
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

Thx but, this isn't wrong, this is using usual CIDR syntax.

As this is just an example of what it does, I think it is better to stay that way because this is understandable by people that doesn't fully grasp multiaddr yet,
if you want you can add an example of multiaddr syntax.

@alx696
Copy link
Author

alx696 commented Nov 8, 2022

Thx but, this isn't wrong, this is using usual CIDR syntax.

As this is just an example of what it does, I think it is better to stay that way because this is understandable by people that doesn't fully grasp multiaddr yet, if you want you can add an example of multiaddr syntax.

Sorry for my inaccurate description. A directly usable example might be better for starter. In this way, the configuration can be completed even if the professional knowledge is insufficient.

@alx696 alx696 closed this Nov 8, 2022
@alx696 alx696 deleted the patch-1 branch November 8, 2022 01:15
@Jorropo
Copy link
Contributor

Jorropo commented Nov 8, 2022

Sorry for my inaccurate description. A directly usable example might be better for starter.

I agree, but I would like to keep the existing thing for thoses who know it, an example of how this could look like:

(e.g., 192.168.0.0/16 or in mutladdr form /ip4/192.168.0.0/ipcidr/16)

@decentralgabe
Copy link
Contributor

This should be re-opened and merged. The current example is confusing and not helpful!

@Jorropo
Copy link
Contributor

Jorropo commented Feb 21, 2023

The current example is confusing and not helpful!

I overlooked this, sorry.

One possible solution would be to edit this paragraph to be:

-preventing dials to all non-routable IP addresses (e.g., `192.168.0.0/16`) but
+preventing dials to all non-routable IP addresses (e.g., `/ip4/192.168.0.0/ipcidr/16`
+which is the multiaddress representation of `192.168.0.0/16`) but

However I think this is a little weird because in this sentence:

(e.g., 192.168.0.0/16)

Does not talk about the config direclty, it refers to:

non-routable IP addresses

Which refers to stuff you would want to add into the config, the connection between 192.168.0.0/16 and /ip4/192.168.0.0/ipcidr/16 seems rather indirect to me.

So an other solution could be to add a new example paragraph showing you the syntax or a list of examples.
I don't have a preference between boths.

If someone opens a PR implementing either of both solution I'll merge it (or try something else if you think you have a better idea). 🙂

@decentralgabe
Copy link
Contributor

Hi @Jorropo I opened #9661

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