stellar / js-xdr

Read/write XDR encoded data structures (RFC 4506)
Apache License 2.0
25 stars 31 forks source link

Add a robustier `instanceof` alternative to allow cross-package compatibility. #122

Closed Shaptic closed 3 months ago

Shaptic commented 6 months ago

Note: Lots of people tagged for review mostly as an FYI as there may or may not be far-reaching implications to this change.

What

Introduce a better check for cross-compatible XdrCompositeType subtypes (Union, Struct, Enum, and Array) based on a Discord discussion from @earrietadev which will check the prototype chain for the constructors and properties we need:

export function isSerializableIsh(
    value: any, 
    subtype: InstanceType<typeof XdrCompositeType>
): value is XdrCompositeType;

(This isn't a proper TypeScript definition because js-xdr (a) doesn't use TS and (b) doesn't use interfaces, but imagine XdrCompositeType as being an interface and it makes more sense.)

We also make the error messages way better.

Details

Because the classes are generated, their prototype constructor name doesn't match the actual class name more often than not. However, they do still inject the "true" name into their prototype. The three types we're trying to match on all do this differently:

It's not perfect, but it's better than the instanceof check we have now.

So we will check these fields as well as the existence of a read() and write() method to make a best-effort guess that the given value matches the kind we expect.

Why

Major versions of a package should probably be cross-compatible in the same dependency tree. For example, js-xdr@3.1.1 should function interchangeably alongside js-xdr@3.0.0 if the additive features are not relevant.

That is not the case today due to instanceof checks that become package-specific, while isSerializableIsh should rectify this.

Known Limitations

There are only performance implications for people who are introducing this mixing behavior (besides the new overhead of a function call which I would consider trivial), so there are no performance regression risks.

BlaineHeffron commented 6 months ago

The new check also fails to resolve https://github.com/stellar/soroban-cli/issues/1285

piyalbasu commented 4 months ago

This looks solid to me. I'll echo @willemneal 's thought about using the Class rather than this. Probably not super critical, but you may as well cover all your bases

Shaptic commented 4 months ago

I've tried testing this with downstream systems and landed upon the wonderfully helpful "[object Object]'s struct name is SorobanAuthorizationEntry not SorobanAuthorizationEntry: " so clearly the extra check is still failing in key moments. I'm still in the process of debugging it and will merge this once I can pull that off.

Shaptic commented 3 months ago

Yooooo I ran this through a project that uses vite all the way down through the SDK and it works!! :partying_face: :partying_face: