-
Notifications
You must be signed in to change notification settings - Fork 687
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
sdk/js: handling valid exceptions #3590
Conversation
Signed-off-by: Abhishekkochar <[email protected]>
@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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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")){ |
There was a problem hiding this comment.
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?
There was a problem hiding this 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?
@kcsongor, Can you please confirm if this is correct algogrand/package? |
I have tried to run them. There is an error when running alogrand tests? |
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. |
fix: #3190