Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

Allow for graceful failure of Promise.all if one of many queries reverts / errors. #56

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0xidkfa
Copy link

@0xidkfa 0xidkfa commented Apr 24, 2023

feat(ethers.ts): improved wrap function handles query failures gracefully and returns error object

BREAKING CHANGE: Wrap no longer throws an error.

When calling Promise.all([wrappedUni.symbol(), wrappedUnknown.symbol()]), the current library will throw an error for the entire promise. Instead of throwing an error, I've made a slight modification to return an error object. The response will now come in the following form:

[
        "UNI",
        {
          address: "0xd6409e50c05879c5B9E091EB01E9Dd776d00A151",
          error: new Error(
            "0xd6409e50c05879c5B9E091EB01E9Dd776d00A151:symbol() empty return data exception"
          ),
          params: [],
        }
]

Pull-Request Checklist

  • Code is up-to-date with the main branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • [N/A] This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • [N/A] Documentation has been updated to reflect this change
  • The new commits follow conventions outlined in the conventional commit spec

…ully and returns error object

BREAKING CHANGE: Wrap no longer throws an error.
@0xidkfa
Copy link
Author

0xidkfa commented Apr 24, 2023

@Rubilmax Is there a better / different way to do this? I noticed in 4.0.0, you already tried to add graceful failure, but I couldn't figure out how to get away with it when using Promise.all...

@Rubilmax
Copy link
Collaborator

@Rubilmax Is there a better / different way to do this? I noticed in 4.0.0, you already tried to add graceful failure, but I couldn't figure out how to get away with it when using Promise.all...

Hi @0xidkfa, thanks for your contribution!
It seems it's not the best way to do it, as handling errors would require promises to be independent. Unfortunately, they are with the current design of this package and would require an entire refactoring.

Fortunately, this refactoring was performed in the new package ethers-multicall-provider, that you can find here: https://github.com/rubilmax/ethers-multicall-provider

We shall decommission this package now that the new one replaced it with many new & useful features

@0xidkfa
Copy link
Author

0xidkfa commented Apr 24, 2023

Thanks for this! Is this package deprecated in favor of the ethers-multicall-provider package? If not, what are the key differences between the two?

Also, when you say promises are not independent in this library, do you mind giving an example of when promises would be dependent on each other?

@Rubilmax
Copy link
Collaborator

Thanks for this! Is this package deprecated in favor of the ethers-multicall-provider package? If not, what are the key differences between the two?

Indeed, I'll deprecate this package in favor of ethers-multicall-provider!

Also, when you say promises are not independent in this library, do you mind giving an example of when promises would be dependent on each other?

Using this package, if any of the following contract calls revert, all others revert too and you don't get a good explanation of what fails:

await Promise.all([
  wrappedUni.symbol(),
  wrappedUni.unknown().catch(),
]);

so you will never successfully get the symbol because the unknown function will always revert (even if you catch it, because this package bundles all promises performed within a 10ms window into a single one, via dataloader)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants