SVS doesn't work with Name as ID

pulsejet commented 3 years ago

The following doesn't work as expected (no updates are received), but works fine when a string is used instead of a name in sync.add. Is this expected?

const sync = new SvSync({
    syncPrefix: new Name("/ndn/multicast/svs"),
    endpoint: endpoint,
})
const syncNode = sync.add(new Name(`/ndn/alice/svs`)); // <-- sync.add(`/ndn/alice/svs`) works.

Reproduce on NDN Play. (A,B) run a Sync group using Name and (C,D) run a separate group using string. (running on nightly build for 10/28/2021)

yoursunny commented 3 years ago

StateVectorSync Protocol Specification version 2021-02-23 defines NodeID to be a byte array, not a name.

NodeID = VERSION-VECTOR-KEY-TYPE TLV-LENGTH *OCTET

NDNts @ndn/sync accepts either a string or a byte array as SvSync ID. The typing is:

class SvSync {
    get(id: IDLike): SyncNode<ID>;
    add(id: IDLike): SyncNode<ID>;
    // other members
}

type IDLike = string | Uint8Array | ID;

If you pass a Name as SvSync ID, TypeScript compiler will report a type error. This becomes an undefined behavior if type checking is not enforced.

This design is consistent with ndn-svs C++ library, which only accepts std::string as ID: https://github.com/named-data/ndn-svs/blob/a5dee089253f055e553081cb202cc6da108d9d12/ndn-svs/common.hpp#L37

I've seen libraries using URI representation of a name when a byte array is needed:

This leads to portability concerns because URI representation may change over time. Therefore, I do not follow such bad practice.

pulsejet commented 3 years ago

Ah, sorry my fault. The new specification of SVS will change the ID to a Name (implemented at the develop branch in the C++ lib).

Also that means I need to fix the typescript compiler in NDN Play 🤡

yoursunny / NDNts

SVS doesn't work with Name as ID #7