-
Notifications
You must be signed in to change notification settings - Fork 379
Support promise inputs in ethers-v5 target #700
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
Conversation
🦋 Changeset detectedLatest commit: f619c59 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
huh, i am somehow surprised that ethers.js supports promises instead of values :O We will need runtime tests for: a mocha test case where you would pass value and then pass promise of that value etc to test if this is really a case in runtime and to prevent regressions. |
| it('promise works', async function () { | ||
| const { contract } = chain | ||
|
|
||
| typedAssert(await chain.contract.input_uint8(getPromise('42')), 42) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@krzkaczor would typedAssert also execute the code here and compare runtime values? (I assumed that, but any way pls lmk if we need more tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw yeah its a very underrated/underused thing in ethers that it resolves promise of values passed. Once I just passed a promise while in REPL, when I hit return I realised ahh this is going to fail, and the next second saw that it worked ✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this documented somewhere? I would rather avoid us relaying on an undocumented feature which might break in the future because it's not part of the official spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed this with Richard, and he mentioned though the feature is in ethers since v4, it's not much mentioned, and at least for v5 this behavior not gonna be changed.
But I understand your point. You may consider this if you think it adds value. I've made the PR just because when one has await inside await inside await, it looks crazy and the code can look better without them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah if @ricmoo says it's officially supported, then its fine. Thank you for clarifying that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used quite extensively internal to ethers too.
I guess I only ever talked about it during presentations? I didn’t realize it was so secret. :)
Before:
After: