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.

Sign up for GitHub

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

Jump to bottom

feat: update challenge transaction helpers for SEP-0010 v3.0.0. #308

Merged
merged 8 commits into from
Dec 10, 2020
Merged

feat: update challenge transaction helpers for SEP-0010 v3.0.0. #308

merged 8 commits into from
Dec 10, 2020

Conversation

overcat
Copy link
Member

@overcat overcat commented Nov 10, 2020 •
edited by leighmcculloch
Loading

@overcat
feat: add support for SEP-10 v3.0.0.
c32e663
@overcat
Merge branch 'master' into dev-sep10-v3
6b4b517 
# Conflicts:
#	src/main/java/org/stellar/sdk/Sep10Challenge.java
#	src/test/java/org/stellar/sdk/Sep10ChallengeTest.java
@overcat overcat marked this pull request as ready for review November 10, 2020 14:47
JakeUrban
JakeUrban reviewed Nov 12, 2020
View reviewed changes
Copy link
Contributor

@JakeUrban JakeUrban left a 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

src/main/java/org/stellar/sdk/Sep10Challenge.java Outdated Show resolved Hide resolved
@overcat @JakeUrban
fix the error message in SEP-10
b20d850 
Co-authored-by: Jake Urban <10968980+JakeUrban@users.noreply.github.com>
JakeUrban
JakeUrban reviewed Nov 16, 2020
View reviewed changes
Copy link
Contributor

@JakeUrban JakeUrban left a 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 👍

src/main/java/org/stellar/sdk/Sep10Challenge.java Outdated Show resolved Hide resolved
src/main/java/org/stellar/sdk/Sep10Challenge.java Outdated Show resolved Hide resolved
src/main/java/org/stellar/sdk/Sep10Challenge.java Outdated Show resolved Hide resolved
src/main/java/org/stellar/sdk/Sep10Challenge.java Outdated Show resolved Hide resolved
src/main/java/org/stellar/sdk/Sep10Challenge.java Outdated Show resolved Hide resolved
@overcat @JakeUrban
fix the docs in SEP-10
5d52a5f 
Co-authored-by: Jake Urban <10968980+JakeUrban@users.noreply.github.com>
JakeUrban
JakeUrban approved these changes Nov 17, 2020
View reviewed changes
Copy link
Contributor

@JakeUrban JakeUrban left a comment

Choose a reason for hiding this comment

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

LGTM!

@overcat overcat mentioned this pull request Dec 7, 2020
Merged
leighmcculloch
leighmcculloch suggested changes Dec 9, 2020
View reviewed changes
Copy link
Contributor

@leighmcculloch leighmcculloch left a 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.

CHANGELOG.md Outdated Show resolved Hide resolved
src/test/java/org/stellar/sdk/Sep10ChallengeTest.java Outdated
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());
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Contributor

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.

JakeUrban
JakeUrban approved these changes Dec 9, 2020
View reviewed changes
Copy link
Contributor

@JakeUrban JakeUrban left a 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

@leighmcculloch leighmcculloch self-assigned this Dec 9, 2020
overcat and others added 2 commits December 10, 2020 09:36
leighmcculloch
leighmcculloch approved these changes Dec 10, 2020
View reviewed changes
Copy link
Contributor

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

🎉 Thanks @overcat !

@leighmcculloch leighmcculloch merged commit 471715f into lightsail-network:master Dec 10, 2020
@overcat overcat deleted the dev-sep10-v3 branch August 11, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JakeUrban JakeUrban JakeUrban approved these changes

@leighmcculloch leighmcculloch leighmcculloch approved these changes

Assignees

@leighmcculloch leighmcculloch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SEP-10 v3.0 Changes (from the SDF)
3 participants
@overcat @leighmcculloch @JakeUrban