feat(dds): do not opaquely encode handles in ops

[connorskees]

Update DDSes to not opaquely encode handles as strings, but rather pass around actual objects.

AB#6382

[rajatch5]

@connorskees Is the PR ready for review? If yes, do you wanna update the PR Description to share the context and describe the changes? Would be helpful to read before heading over to the file.

[rajatch5]

Thanks for updating @connorskees . A little more context would help someone unfamiliar with the area. For instance, what does opaquely encode handles as string means? What's the problem with that? And why are we moving away from it and passing around actual objects now. Who does this whole work help and how?

---

In reply to: 1936543215

[msfluid-bot]

⯅ @fluid-example/bundle-size-tests: +356 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	513.78 KB	513.79 KB	⯅ +10 Bytes
connectionState.js	680 Bytes	680 Bytes	■ No change
containerRuntime.js	247.88 KB	247.9 KB	⯅ +22 Bytes
loader.js	170.65 KB	170.67 KB	⯅ +22 Bytes
map.js	46.53 KB	46.7 KB	⯅ +176 Bytes
matrix.js	148.68 KB	148.73 KB	⯅ +47 Bytes
odspDriver.js	97.3 KB	97.33 KB	⯅ +33 Bytes
odspPrefetchSnapshot.js	42.27 KB	42.29 KB	⯅ +22 Bytes
sharedString.js	167.42 KB	167.32 KB	⯆ -111 Bytes
sharedTree.js	334.41 KB	334.53 KB	⯅ +124 Bytes
Total Size	1.87 MB	1.87 MB	⯅ +356 Bytes

---

Baseline commit: 9356309db03799d5860b9df597426f0d4eb8add0

Generated by :no_entry_sign: dangerJS against f7cdc56f869e81d7349d8d9aace83f371f8a98cf

[agarwal-navin]

@connorskees @markfields This is a risky change. We should ensure that are enough tests in place to not break anything. The scenario where a combination of new and old runtime version clients are running in parallel should be validated. We should add basic end-to-end tests where there are 2 containers - one writes and other reads and run it in full compat mode so it does the mutli-client version compat testing.

[markfields]

@agarwal-navin -- I agree it is risky. Without any type validation to distinguish between encoded v. decoded handles, it's nearly impossible to tell whether all the flips/flops between those states have been addressed correctly. In fact, I know of one still in the PR that hasn't, I've logged a follow-up item.

The reason I'm not overly concerned is that functions like makeHandlesSerializable and parseHandles are idempotent - in terms of correctness, all that matters is that on the way out, the handles are encoded somewhere and on the way in it's the inverse. Seeing that we have moved coverage here to the SharedObject choke points, I feel confident that outside the DDS layer there's not going to be any behavior change here. (inside the DDS layer is another story... counting on existing test coverage here)

So as for this:

The scenario where a combination of new and old runtime version clients are running in parallel should be validated

I think we have a pretty good argument that there's no change over the wire.

That said, better safe than sorry, so the test you suggested is a good idea. I wonder if we can find an existing test that is doing this incidentally?

@connorskees what do you think?

[markfields]

(Disabling auto-merge until @connorskees has a chance to respond to @agarwal-navin)