Handle scSpecTypeVal in Spec.nativeToScVal for raw JS types - #1485

Hey @Shadow-MMN, thanks for taking this on! I'd like to see changed before merging though:

1. Most of this already exists in the SDK — let's delegate instead of duplicating.

We already have a generic typeless converter: nativeToScVal in src/base/scval.ts. With no options it does basically everything this branch hand-rolls: strings, numbers/bigints via ScInt, booleans, undefined/null → void, arrays, sorted maps with string keys, Address/Contract/Uint8Array/ScVal passthrough, and it rejects class instances. So the whole scSpecTypeVal branch could pretty much just be:

if (value === xdr.ScSpecType.scSpecTypeVal().value) {
  return nativeToScVal(val);
}

The only thing it doesn't handle is Map — I'd add that to the base helper instead, so the two code paths can't drift apart over time.

2. I'd drop the address-guessing on strings.

It makes the output type depend on the string's content — any strkey that happens to checksum becomes scvAddress, everything else becomes scvString. Address.fromString also accepts B.../L... strkeys, which aren't valid contract inputs. This is why the base helper makes you opt in with {type: 'address'}. If someone wants an address they can just pass an Address object — no ambiguity.

Smaller stuff:

• val.constructor === Map misses Map subclasses; instanceof Map is safer.
• The guard change at the bottom of the object case (constructor?.name → Object.prototype.toString.call) doesn't change what throws — both paths still error — but it does route class instances to the vaguer "did not match the provided type" message instead of the more helpful "cannot interpret Foo value as ScVal". If the goal was handling null-prototype objects, the getPrototypeOf check you use in the Val branch is the cleaner fix.

The nice thing is your test suite should carry over basically unchanged after the restructure, and that's the hard part. Happy to take another look once it's updated!

Hey @Shadow-MMN, thanks for taking this on! I'd like to see changed before merging though:

1. Most of this already exists in the SDK — let's delegate instead of duplicating.

We already have a generic typeless converter: nativeToScVal in src/base/scval.ts. With no options it does basically everything this branch hand-rolls: strings, numbers/bigints via ScInt, booleans, undefined/null → void, arrays, sorted maps with string keys, Address/Contract/Uint8Array/ScVal passthrough, and it rejects class instances. So the whole scSpecTypeVal branch could pretty much just be:

if (value === xdr.ScSpecType.scSpecTypeVal().value) {
  return nativeToScVal(val);
}

The only thing it doesn't handle is Map — I'd add that to the base helper instead, so the two code paths can't drift apart over time.

2. I'd drop the address-guessing on strings.

It makes the output type depend on the string's content — any strkey that happens to checksum becomes scvAddress, everything else becomes scvString. Address.fromString also accepts B.../L... strkeys, which aren't valid contract inputs. This is why the base helper makes you opt in with {type: 'address'}. If someone wants an address they can just pass an Address object — no ambiguity.

Smaller stuff:

• val.constructor === Map misses Map subclasses; instanceof Map is safer.
• The guard change at the bottom of the object case (constructor?.name → Object.prototype.toString.call) doesn't change what throws — both paths still error — but it does route class instances to the vaguer "did not match the provided type" message instead of the more helpful "cannot interpret Foo value as ScVal". If the goal was handling null-prototype objects, the getPrototypeOf check you use in the Val branch is the cleaner fix.

The nice thing is your test suite should carry over basically unchanged after the restructure, and that's the hard part. Happy to take another look once it's updated!

Thank you for the detailed review, will be pushing a change to address them.