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

E(userSeat).getPayout('NoSuchKeyword') result violates documented types #8610

Open
gibson042 opened this issue Dec 4, 2023 · 2 comments · May be fixed by #10738
Open

E(userSeat).getPayout('NoSuchKeyword') result violates documented types #8610

gibson042 opened this issue Dec 4, 2023 · 2 comments · May be fixed by #10738
Assignees
Labels
bug Something isn't working triaged_2024 Zoe package: Zoe

Comments

@gibson042
Copy link
Member

Originally posted by @gibson042 in #8597 (comment)

P.S. getPayout(keyword) is typed to return a promise for a Payment, but the implementation can return a promise for undefined in cases where the argument is not a contract-defined keyword or isn't even a keyword at all (e.g., await E(seat).getPayout('notAKeyword')).

#8597 (comment)

Maybe that's an argument that getPayout() should throw rather than returning a promise for undefined. The type system is certainly not smart enough to understand that if your argument is right, you shouldn't have to check for undefined.

@Chris-Hibbert
Copy link
Contributor

I tried to create a PR (#10738) to address this, but the fixes cascaded without end. Most of the code that ended up out of compliance was in tests, where failure would be detected appropriately. But adding the improvements to the types would have required adding // @ts-expect-error ts says payment might be null in hundreds of places.

The few places where the changes would have been in production code mostly would have added an assertion that would have caught the error a few lines early, but it would have already been handled cleanly.

I'll close these PRs and Issues in a few days if there's no outcry or suggestions for a better solution.

cc: @markm, @turadg

@turadg
Copy link
Member

turadg commented Dec 28, 2024

We should do our best to make the types accurate. If so much code assumes the promise must resolve to a Payment, and we deem it too much effort to correct them, we should consider changing the implementation so it works as documented.

returns a promise for the Payment corresponding to the indicated keyword

We can make it throw when it can't return a promise for a payment. In cases were the caller isn't certain that the keyword exists, it can use getPayouts() instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triaged_2024 Zoe package: Zoe
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants