search

Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
327 stars 207 forks source link

Async Flow types don't sufficiently reflect the membrane wrapping #9822

Open mhofman opened 3 months ago

mhofman commented 3 months ago

Describe the bug

The HostOf and GuestOf type helpers don't map the argument types, nor do they correctly map non passable types.

See https://github.com/Agoric/agoric-sdk/pull/9797#pullrequestreview-2211524054 for some examples

Somewhat related, prepareEndowment doesn't have correct types.

Expected behavior

Types faithful to the membrane mapping, including never types when a mapping would throw or panic the membrane

turadg commented 3 months ago

https://github.com/Agoric/agoric-sdk/compare/9822-HostOf has some of the suggestions but some of the strictness explodes. e.g. unknown does not pass as Passable.

None of the changes fix any ts-expect-error so I haven't pushed it as a PR. I think the test plan for this work should include new unit tests in a test-d.ts. I see there's no Test Plan section because this was filed as a bug, but fwiw I consider it an enhancement to the type checking. There has been no claim that the types do any more than the shortcomings described.

mhofman commented 3 months ago

Thanks for that draft branch. Whether to classify incorrect types as a bug or an enhancement is debatable. Without tests or cases highlighting the issue, I agree it's hard, and there should definitely be some added as part of this work.

Regarding how to treat unknown, I guess it's a larger question. I kinda wish unknown was "Passable" in the sense that it would infect the other side. The alternative is to transform an unknown into Passable, which feels incorrect. I'll bring this topic up for discussion

mhofman commented 1 month ago

@michaelfig and I will help @turadg work on fixing these types once the tests in #9933 are in place