Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(smart-wallet): don't try to distinguish presences statically #8120

Merged
merged 2 commits into from
Aug 1, 2023

Conversation

dckc
Copy link
Member

@dckc dckc commented Jul 31, 2023

refs: #7226, #8071

Description

Using static types to distinguish a Purse from a presence-for-a-Purse (likewise Payment) is not common in our code; the attempt to do it here seems to cause unwarranted friction.

Security...Testing Considerations

none; types only

Using static types to distinguish a Purse from a presence-for-a-Purse
(likewise Payment) is not common in our code; the attempt to do it
here seems to cause unwarranted friction.
@dckc dckc requested a review from turadg July 31, 2023 19:51
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little sad to see this loss of specificity but we don't have the types support yet to make it practical.

@erights
Copy link
Member

erights commented Jul 31, 2023

distinguish a Purse from a presence-for-a-Purse

A little sad to see this loss of specificity but we don't have the types support yet to make it practical.

@michaelfig , I thought you do. Did I misunderstand?

@michaelfig
Copy link
Member

distinguish a Purse from a presence-for-a-Purse

A little sad to see this loss of specificity but we don't have the types support yet to make it practical.

@michaelfig , I thought you do. Did I misunderstand?

I think they're all expressible with the types from @endo/far:

  • a Purse is just Purse
  • a presence-for-a-Purse is import('@endo/far').RemotableBrand<{}, Purse>
  • one of the above is Awaited<import('@endo/far').FarRef<Purse>>
  • either above or promises thereof is import('@endo/far').FarRef<Purse>.

@dckc
Copy link
Member Author

dckc commented Aug 1, 2023

A little sad to see this loss of specificity but we don't have the types support yet to make it practical.

@michaelfig , I thought you do. Did I misunderstand?

I think they're all expressible with the types from @endo/far:
...

ok, that's more than I knew.

But ERTP just uses Purse and Payment. Until ERTP uses these fine distinctions, there's friction in trying to use them here. For example, if I try to use static types in this package to express that a payment should be a payment or a presence for one...

/**
 * @param {ERef<Purse>} purse
 * @param {Awaited<import('@endo/far').FarRef<Payment>>} pmt
 */
const funWithTypes = async (purse, pmt) => {
  return E(purse).deposit(pmt);
};

I get an error:

error TS2345: Argument of type 'DataOnly<Payment<AssetKind>> & RemotableBrand<DataOnly<Payment<AssetKind>>, Payment<AssetKind>>' is not assignable to parameter of type 'Payment<AssetKind>'.
  Property 'getAllegedBrand' is missing in type 'DataOnly<Payment<AssetKind>> & RemotableBrand<DataOnly<Payment<AssetKind>>, Payment<AssetKind>>' but required in type 'Payment<AssetKind>'.

43   return E(purse).deposit(pmt);

It reminds me of trying to use ReadOnly<T> for the result of harden(). It would be nice, but given the surrounding context, it results in a lot of friction.

@dckc
Copy link
Member Author

dckc commented Aug 1, 2023

One thing that I think makes using this sort of distinction awkward is: it's not a pass-invariant property. In E(purse).deposit(pmt1), the pmt1 is probably local to the receiver but remote to the caller.

Maybe there's some that the type for E could morph the types of all the arguments to line up.

But normally, the caller and the callee in an API use the same type.

@michaelfig
Copy link
Member

funWithTypes

What I'm suggesting would only work by changing the typing on all deposit(pmt)-like functions in ERTP to use Awaited<FarRef<Payment>> to correctly specify their "non-promise, either remote or local Payment" constraint.
Those are not quite as far-reaching as readonly-harden, mainly because requiring a local-or-presence argument is an exceptional case.

@dckc
Copy link
Member Author

dckc commented Aug 1, 2023

The constraints on what you can pass to deposit() are one thing that the static types in this package were trying to do. I'm already satisfied that ERTP says .deposit() takes a Payment and not an ERef<Payment>. That provides the sort of feedback I'm interested in.

But another motivation was synchronous calls such as purse.getCurrentAmount(). They worked in ava tests, but I flagged them during review since purse is not in the same vat in production. Turadg's efforts to use static typing to prevent such problems is a nifty goal, but until we find something that works more widely, the few constraints here seem like more trouble than they're worth.

@dckc dckc enabled auto-merge August 1, 2023 17:12
@dckc dckc added this pull request to the merge queue Aug 1, 2023
Merged via the queue into master with commit 50332a8 Aug 1, 2023
@dckc dckc deleted the dc-wallet-remote-types branch August 1, 2023 18:11
mhofman pushed a commit that referenced this pull request Aug 7, 2023
chore(smart-wallet): don't try to distinguish presences statically
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants