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

[v6] FetchCancelSignal might have an incorrect assertion #3733

Closed
hzhu opened this issue Feb 4, 2023 · 5 comments
Closed

[v6] FetchCancelSignal might have an incorrect assertion #3733

hzhu opened this issue Feb 4, 2023 · 5 comments
Assignees
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published. v6 Issues regarding v6

Comments

@hzhu
Copy link

hzhu commented Feb 4, 2023

Ethers Version

6.0.0

Search Terms

provider

Describe the Problem

Hi! Thank you for shipping V6 🥳. Before I submit a pull request, I wanted to confirm if my understanding of the issue is correct. The provider is unable to fetch:

const RPC_URL = "https://eth-mainnet.g.alchemy.com/v2/<API-KEY>";
const provider = new JsonRpcProvider(RPC_URL);
const network = await provider.getNetwork(); // can't fetch network 😢 

✅ Minimal CodeSandbox demo in v5 (working)

❌ Minimal CodeSandbox demo in v6 (not working)

The error

Error: signal already cancelled (operation="fetchCancelSignal.addCancelListener", code=UNSUPPORTED_OPERATION, version=6.0.0)

I tested this locally and received an assertion error from utils/fetch.ts claiming that the signal was cancelled, however it wasn't (this.#cancelled was set to false).

It's coming from the assertion in addListener:

addListener(listener) {
  assert$1(this.#cancelled, "singal already cancelled", "UNSUPPORTED_OPERATION", {
    operation: "fetchCancelSignal.addCancelListener"
  });
  this.#listeners.push(listener);
}

The solution

As the assert function inverts the check boolean internally, perhaps it should be rewritten in this manner:

addListener(listener) {
  if (this.#cancelled) {
    assert$1(false, "signal already cancelled", "UNSUPPORTED_OPERATION", {
        operation: "fetchCancelSignal.addCancelListener"
    });
  } else {
    this.#listeners.push(listener);
  }
}

Here's a live demo of the solution. Here's a CodeSandbox demo of the solution in practice. I copied Ethers v6 from Cloudflare CDN to a local file and adjusted the addListener function based on the solution mentioned above.

Code Snippet

No response

Contract ABI

No response

Errors

Error: signal already cancelled (operation="fetchCancelSignal.addCancelListener", code=UNSUPPORTED_OPERATION, version=6.0.0)

The "listener" argument must be of type Function. Received type object

Environment

Browser (Chrome, Safari, etc)

Environment (Other)

No response

@hzhu hzhu added the investigate Under investigation and may be a bug. label Feb 4, 2023
@ricmoo
Copy link
Member

ricmoo commented Feb 4, 2023

Ack! I switched a bunch of if (blah) throw to assert(!blah) and missed some of the negations. I thought I’d caught them all, but might have missed this one.

I’ll get to this right away. Thanks!

@ricmoo ricmoo added on-deck This Enhancement or Bug is currently being worked on. v6 Issues regarding v6 labels Feb 4, 2023
@ricmoo
Copy link
Member

ricmoo commented Feb 4, 2023

So, the signal being crossed is definitely a bug, but not one that should be affecting your above code.

I am able to use your code to get back Network object...

@ricmoo
Copy link
Member

ricmoo commented Feb 4, 2023

Oh!! Maybe because you are in the browser? Which uses a different geturl function, which does use the Signal.

Ok... I'll make the change and push it out, because it is certainly a bug. :)

@ricmoo ricmoo added bug Verified to be an issue. and removed investigate Under investigation and may be a bug. labels Feb 4, 2023
ricmoo added a commit that referenced this issue Feb 4, 2023
@ricmoo ricmoo added fixed/complete This Bug is fixed or Enhancement is complete and published. and removed on-deck This Enhancement or Bug is currently being worked on. labels Feb 4, 2023
@ricmoo
Copy link
Member

ricmoo commented Feb 4, 2023

This should be fixed in v6.0.2. Try it out and let me know! :)

This is also extra incentive to get the Karma tests back up an running, to help detect the browser-specific issues.

Thanks! :)

@hzhu
Copy link
Author

hzhu commented Feb 4, 2023

It's working now! Your fix was incredibly fast and I am so grateful. Thank you so much @ricmoo!

@hzhu hzhu closed this as completed Feb 4, 2023
Woodpile37 pushed a commit to Woodpile37/ethers.js that referenced this issue Jan 14, 2024
Woodpile37 pushed a commit to Woodpile37/ethers.js that referenced this issue Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published. v6 Issues regarding v6
Projects
None yet
Development

No branches or pull requests

2 participants