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: handling valid exceptions #3590

Conversation

Abhishekkochar
Copy link

fix: #3190

Signed-off-by: Abhishekkochar <abhishekkochar2@gmail.com>
@Abhishekkochar
Copy link
Author

@kcsongor, Please have a look.

@@ -127,7 +127,11 @@ describe("Algorand tests", () => {
vaaBytes
);
} catch (e) {
success = false;
if(e instanceof Error && e.message.includes("wrapped asset already exists")){
success = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be the other way around?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes. Fixed. Will push soon.

success = false;
//@notice
if(e instanceof Error && e.message.includes("wrapped asset already exists")){
success = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one too

@@ -420,6 +425,11 @@ describe("Aptos SDK tests", () => {
await createWrappedOnEth(ethTokenBridge, recipient, attestVAA);
} catch (e) {
// this could fail because the token is already attested (in an unclean env)
if(e instanceof Error && e.message.includes("wrapped asset already exists")){
Copy link
Contributor

@kcsongor kcsongor Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the error message actually look like this?

Copy link
Contributor

@kcsongor kcsongor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you run these tests locally?

@Abhishekkochar
Copy link
Author

@kcsongor, Can you please confirm if this is correct algogrand/package?
There is no test.ts file in test dir.

@Abhishekkochar
Copy link
Author

Did you run these tests locally?

I have tried to run them. There is an error when running alogrand tests?

@evan-gray evan-gray added the sdk label Mar 14, 2024
@evan-gray
Copy link
Contributor

This error case is only hit when the tests are run a second time on an existing tilt instance. This never happens in CI and is only a hinderance to a developer trying to run the tests without cycling the corresponding chains. Given that the subsequent test cannot pass if the token was not properly attested, the existing code is adequate for CI. So, for the benefit of devs in this repo, it seems a more prudent thing would be to skip the test step altogether if the token was already attested.

@evan-gray evan-gray closed this Mar 14, 2024
@Abhishekkochar Abhishekkochar deleted the abhishek/issue3190 branch March 28, 2024 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sdk/js: catch-all catch blocks swallow exceptions
3 participants