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

Add memo field to Sep10Challenge.newChallenge() #435

Merged
merged 1 commit into from
May 27, 2022
Merged

Add memo field to Sep10Challenge.newChallenge() #435

merged 1 commit into from
May 27, 2022

Conversation

lijamie98
Copy link
Contributor

No description provided.

@lijamie98 lijamie98 requested a review from tamirms May 26, 2022 03:04
@lijamie98 lijamie98 requested a review from JakeUrban May 26, 2022 03:09
JakeUrban
JakeUrban suggested changes May 26, 2022
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.

We also need to validate that the memo is of type ID in the read challenge tx function.

src/main/java/org/stellar/sdk/Sep10Challenge.java Show resolved Hide resolved
@lijamie98 lijamie98 force-pushed the add-memo-to-new-challenge branch 2 times, most recently from 42fc11a to cdf3630 Compare May 26, 2022 11:22
JakeUrban
JakeUrban approved these changes May 26, 2022
View reviewed changes
Copy link
Contributor

@JakeUrban JakeUrban left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This PR looks good for the newChallenge() method, but I think we should also update readChallengeTx() to verify the memo attached, if present, is of type ID. Maybe that makes more sense in a 2nd PR, I'll leave it to you.

@tamirms
Copy link
Contributor

tamirms commented May 27, 2022

I agree with @JakeUrban , should we be validating the memo field during readChallengeTx?

@lijamie98
Copy link
Contributor Author

I agree with @JakeUrban , should we be validating the memo field during readChallengeTx?

I had added memo validation to the readChallengeTx().

@lijamie98
Add memo field to Sep10Challenge.newChallenge()
c087775 
Add memo type 'id' check to readChallengeTransaction()
@lijamie98 lijamie98 merged commit d364100 into master May 27, 2022
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

@tamirms tamirms Awaiting requested review from tamirms

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lijamie98 @tamirms @JakeUrban