-
Notifications
You must be signed in to change notification settings - Fork 159
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
feat: update challenge transaction helpers for SEP-0010 v3.0.0. #308
feat: update challenge transaction helpers for SEP-0010 v3.0.0. #308
Conversation
# Conflicts: # src/main/java/org/stellar/sdk/Sep10Challenge.java # src/test/java/org/stellar/sdk/Sep10ChallengeTest.java
9d55f04
to
8b8fa49
Compare
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.
One comment but otherwise LGTM
Co-authored-by: Jake Urban <[email protected]>
05e88f2
to
b20d850
Compare
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.
Suggestions mainly about plural vs. singular home domains depending on the function signature. The code is good 👍
Co-authored-by: Jake Urban <[email protected]>
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.
LGTM!
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.
We missed merging this, and I was just about to merge, but did a double check review since it looks like the PR has some force-pushes since I last looked, and I noticed one thing that I think needs changing. It's somewhat minor, but I think we should change.
Sep10Challenge.readChallengeTransaction(transaction.toEnvelopeXdrBase64(), server.getAccountId(), Network.TESTNET, new String[]{}); | ||
fail(); | ||
} catch (InvalidSep10ChallengeException e) { | ||
assertEquals("The transaction's operation key name does not include one of the expected home domains.", e.getMessage()); |
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.
❗ I think this exception should actually be an InvalidArgumentException
. The issue is the calling code didn't provide a domain name in the list, and not that the challenge is missing any specific data.
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.
I think the one exception is if the list is empty like it is in this test. But if a non-empty list is passed, it should be an InvalidSep10ChallengeException
.
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.
Hi, I added additional verification for it and thrown an IllegalArgumentException
exception.
According to the discussion, should IllegalArgumentException
exception be thrown in the code?
https://github.com/stellar/java-stellar-sdk/blob/7b3d60e37c/src/main/java/org/stellar/sdk/Sep10Challenge.java#L86
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.
Yes probably, but let's not change that in the pr since it's unrelated and would be a breaking change to fix. We should fix it in another pr and consider how to handle the breaking change. It might be fine if the SDKs major version is still 0.
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 looks good to me @leighmcculloch
Co-authored-by: Leigh McCulloch <[email protected]>
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.
🎉 Thanks @overcat !
Close #306