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

sdk/js: catch-all catch blocks swallow exceptions #3190

Open
kcsongor opened this issue Jul 12, 2023 · 8 comments
Open

sdk/js: catch-all catch blocks swallow exceptions #3190

kcsongor opened this issue Jul 12, 2023 · 8 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@kcsongor
Copy link
Contributor

kcsongor commented Jul 12, 2023

Description and context

In several places, such as integration tests

try {
await createWrappedOnEth(ethTokenBridge, recipient, attestVAA);
} catch (e) {
// this could fail because the token is already attested (in an unclean env)
}

but even in core sdk functions
} catch (e: any) {
// redeemed if the VAA was already executed
return e.response.data.message.includes("VaaAlreadyExecuted");
}

the catch blocks just swallow the exceptions. This is in most cases anticipating a specific exception, so the catch block should just check for that, and rethrow in any other case.

A good example of a catch block that does the correct thing is

} catch (e: any) {
// redeemed if the VAA was already executed
if (e.response.data.message.includes("VaaAlreadyExecuted")) {
return true;
} else {
throw e;
}
}

Definition of done

No catch-all catch blocks in the sdk.

@kcsongor kcsongor added the good first issue Good for newcomers label Jul 12, 2023
@AlberErre
Copy link
Contributor

Happy to help with this 👀 @kcsongor

could you assign me the issue? 🏄‍♂️

@aadam-10 aadam-10 added this to the Maintenance - SDK milestone Jul 27, 2023
@ritvij14
Copy link

@AlberErre are you still working on this or can I take it up?

@AlberErre
Copy link
Contributor

Hey @ritvij14 feel free to pick this up 👍, I think @kcsongor could assign you the issue 👀

@ritvij14
Copy link

@kcsongor could you please assign the issue to me, I will get started on it

@Abhishekkochar
Copy link

@kcsongor If no-one is actively working on this. I can pick this up from here. Cheers

@kcsongor
Copy link
Contributor Author

kcsongor commented Dec 5, 2023

hey @Abhishekkochar, thanks! I don't believe anyone's actively working on this, so I'll assign the issue to you if you're interested in giving it a go

@kcsongor kcsongor assigned Abhishekkochar and unassigned AlberErre Dec 5, 2023
@Abhishekkochar
Copy link

@kcsongor double checking, are non-org people allowed to contribute? Currently getting error 403 when pushing the branch. I have already check the credentials. Thanks heaps.

@kcsongor
Copy link
Contributor Author

kcsongor commented Dec 8, 2023

You might have to fork the repo and push the branch to your own fork, then open a PR from there!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@kcsongor @AlberErre @aadam-10 @Abhishekkochar @ritvij14 and others