[DISCUSSION] Exposing the underlying struct

[bigs]

In my opinion, hiding the underlying multiaddr struct is a hinderance, particularly when working with serializations in golang (looking at flags and encoding/json). Is there any reason we couldn't simply expose the struct and keep its member hidden? This would enable us to implement interfaces that require zero values.

[bigs]

ping @Kubuxu @raulk

Why not -- are you looking for more serializations than the existing MarshalBinary, MarshalText MarshalJSON?

[Stebalien]

So, this is an issue for deserializing because we need a concrete struct to deserialize into. However, I'm not sure what the issue is for serialization.

Note: there's also the tricky fact that we now have two concrete implementations: Component and multiaddr.

[raulk]

Yeah, I've encountered this hindrance in the past. I get the impression the goal was to enable diversity of multiaddr implementations, but I can't see a use case for that.

[Stebalien]

Yeah, I've encountered this hindrance in the past. I get the impression the goal was to enable diversity of multiaddr implementations, but I can't see a use case for that.

IMO, we should implement Multiaddr for net.TCPAddr, etc. However, I'm not sure how well that would work with how we currently use multiaddrs (it would probably lead to a bunch of allocations).

[raulk]

Ah, I see what you mean. Treating Multiaddrs as a canonical format with conversions from/to other formats. Unlike Rust, I don't think this is feasible in Go by making TCPAddr implement Multiaddr from outside the owning package. We could, of course, provide translation util functions, but that's what go-multiaddr-net does already, right?

[Stebalien]

Hm, no, you're right. We could add newtypes but we can't implement it directly (I sometimes blissfully forget about go's type system).

[bigs]

i think if we keep the interface and just expose the strict as well, it should solve our problem

[Stebalien]

@bigs could you explain how this solves the problem? Having to type assert to the concrete type is going to be annoying.

[bigs]

@Stebalien i misspoke earlier when i said serialization was the issue. it's only deserialization.

there's a somewhat prevalent pattern, for example in the datastores, of having something like

NewBadgerDatastore(...) *BadgerDatastore

where * BadgerDatastore is the receiver for methods implementing the Datastore interface. i think we could do that for Multiaddr. we could even expose the concrete type in another package if we wanted to have the name preserved.

being able to to say

var bytes []byte
var addr *multiaddr.Multiaddr
if err := addr.UnmarshalJSON(bytes); err != nil {
  panic(err)
}

makes it possible for users to seamlessly read in json objects with string representations of multiaddrs. we could keep the signature of NewMultiaddr the same if we so pleased. furthermore, any functions accepting the Multiaddr interface should have no problem.

[Stebalien]

So, the issue I have with that is that the struct would need to be:

type Thing struct {
  ID peer.ID
  Addrs []*ma.MultiaddrConcrete
}

But now, when I want to create this thing, I have to convert from my ma.Multiaddr to a *ma.MultiaddrConcrete. Basically, this is going to suck either way: it's either going to be annoying when serializing/deserializing or at a higher layer when creating these structs.

I wonder if using refmt would help? refmt allows specifying explicit transforms so it may be possible to specify how to decode a multiaddr.

[bigs]

ah, you know, that's a fair point. i think it's still probably worth it, because it would only be adding functionality, not removing or hindering anything. that said, it would be wonderful to have some multiaddr magics for refmt! could be a convenience package.

[hsanjuan]

I am going to bump this. I have custom code to wrap multiaddresses into a type that knows how to deserialize itself. Having to do that is worse than anything.

Could I convince you to remove the interface(s) altogether and use a concrete struct type instead, since nowhere this is used as an interface with alternate implementations, and right now it is a pain? (This would be backwards compatible as this type would have all the methods from the interface).

[raulk]

@hsanjuan I'm convinced. Multiaddrs are used everywhere, this indirection has proven useless over time (we have zero alternative implementations), adds a performance cost, and multiaddrs are a data containers per spec, not an abstraction over alternative data structures.

The above use case mentioned by @Stebalien -- using the Multiaddr as a canonical interface to things like TCPAddr -- is not feasible in go, because we can't extend types beyond their definition site. We are going to need translation/wrapper types/methods either way.

I'm very annoyed because the way it's implemented, we are totally unable to unmarshal multiaddrs, for obvious reasons. In fact, the unit tests blissfully use the private type. This is BOA (broken on arrival), in my opinion:

go-multiaddr/multiaddr_test.go

Lines 626 to 629 in 5b1de2f

	var addr2 multiaddr
	if err = addr2.UnmarshalJSON(b); err != nil {
	t.Fatal(err)
	}

Something basic I'd like to be able to do:

	ma, err := multiaddr.NewMultiaddr("/ip4/127.0.0.1/tcp/5001")
	if err != nil {
		panic(err)
	}

	bytes, err := json.Marshal(ma)
	if err != nil {
		panic(err)
	}

	fmt.Println((string)(bytes))

	var addr multiaddr.Multiaddr
	err = json.Unmarshal(bytes, &addr)
	if err != nil {
		panic(err)
	}
	fmt.Println(addr)

"/ip4/127.0.0.1/tcp/5001"
panic: json: cannot unmarshal string into Go value of type multiaddr.Multiaddr