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

Creating a portfolio with a duplicate name error is not catchable. #1116

Closed
po0uyan opened this issue Jan 12, 2024 · 11 comments
Closed

Creating a portfolio with a duplicate name error is not catchable. #1116

po0uyan opened this issue Jan 12, 2024 · 11 comments

Comments

@po0uyan
Copy link

po0uyan commented Jan 12, 2024

Hi, let's consider the following is creating a portfolio which is already there.

try {
      identity?.portfolios.getPortfolio
      const port = await api.identities.createPortfolio({ name: "test" });
      const ress = await port.run();
    } catch (e) {
      console.log("there was an error" + e);
    }

What I expect from this block of code is that If it fails for any reason including a duplicate portfolio name, It would be catchable inside that catch block. But it fails with the following error and doesn't end up in catch block:

throw new internal_1.PolymeshError({
                ^
PolymeshError: There already exist Portfolios with some of the given names
    at Procedure.<anonymous>

can you please guide me through the correct way of handling this?

@polymath-eric
Copy link
Collaborator

Hi @po0uyan, thanks for bringing this to our attention.

The SDK was designed on a previous version of node, where unhandled promises did not crash the process.

This is something we will look into solving more correctly soon, but in the meantime you can use --unhandled-rejections=none as an option for node, or register a handler on the process itself so that the error becomes catchable.

process.on('unhandledRejection', reason => {
  console.warn(`unhandled rejection, reason: ${reason}`);
});

We will be looking into a solution where this isn't needed, but hopefully this works for you in the meantime.

@po0uyan
Copy link
Author

po0uyan commented Jan 12, 2024

Great thanks.

@monitz87
Copy link
Contributor

monitz87 commented Jan 14, 2024

The SDK was designed on a previous version of node, where unhandled promises did not crash the process.

This is something we will look into solving more correctly soon, but in the meantime you can use --unhandled-rejections=none as an option for node, or register a handler on the process itself so that the error becomes catchable.

process.on('unhandledRejection', reason => {
  console.warn(`unhandled rejection, reason: ${reason}`);
});

We will be looking into a solution where this isn't needed, but hopefully this works for you in the meantime.

@polymath-eric

You can use defusePromise from the internal utils when calling prepareTransactions in the Procedure's prepare method to get around the issue. It's ugly, but it was made for this specific scenario.

const prepareTransactionsPromise = defusePromise(this.prepareTransactions(procArgs));

Should be safe since the promise is being handled later on anyway

P.S.: Hi :D

@po0uyan
Copy link
Author

po0uyan commented Jan 17, 2024

unhandledRejection

Thanks @monitz87 , is this available on Polymesh too? can you elaborate more on how I can implement this in creating a portfolio example?
seems like unhandled promise rejection event listener doesn't work on AWS lambda.

@monitz87
Copy link
Contributor

@po0uyan no, sorry for the misunderstanding. My comment was referring to how the bug can be fixed in the SDK code, not to how it can be sidestepped in userland

polymath-eric added a commit that referenced this issue Mar 8, 2024
use `defusePromise` to prevent process crashes from unhandled promise
rejections

✅ Closes: #1116
@polymath-eric
Copy link
Collaborator

@monitz87 Thanks, that seems to resolve the issue!

Sorry for the late reply, I missed the notification you replied.

We've been busy working on the zero knowledge "polymesh-private" stuff which is wrapping up. I figured I would finally have time to tackle this only to find you posted the fix like 6 weeks ago.

I hope things are going well for you!

@polymath-eric polymath-eric self-assigned this Mar 8, 2024
@prashantasdeveloper
Copy link
Collaborator

🎉 This issue has been resolved in version 24.0.0-alpha.15 🎉

The release is available on:

Your semantic-release bot 📦🚀

@polymath-eric
Copy link
Collaborator

@po0uyan You should be able to use the alpha release: 24.0.0-alpha.15 which resolves this issue.

It will get merged into beta and master at a later date.

@prashantasdeveloper
Copy link
Collaborator

🎉 This issue has been resolved in version 24.0.0-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

prashantasdeveloper pushed a commit that referenced this issue Apr 4, 2024
use `defusePromise` to prevent process crashes from unhandled promise
rejections

✅ Closes: #1116
@prashantasdeveloper
Copy link
Collaborator

🎉 This issue has been resolved in version 24.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@prashantasdeveloper
Copy link
Collaborator

🎉 This issue has been resolved in version 23.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

No branches or pull requests

5 participants