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

Restore homeDomain validation #596

Merged
merged 11 commits into from
Nov 9, 2020
Merged

Restore homeDomain validation #596

merged 11 commits into from
Nov 9, 2020

Conversation

quietbits
Copy link
Contributor

No description provided.

quietbits
quietbits commented Nov 5, 2020
View reviewed changes
test/unit/utils_test.js Outdated Show resolved Hide resolved
@quietbits quietbits linked an issue Nov 5, 2020 that may be closed by this pull request
SEP-10 v3.0 Changes (from the SDF) #593
Closed
JakeUrban
JakeUrban requested changes Nov 5, 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.

The biggest piece is the test coverage, otherwise it looks good!

src/utils.ts Outdated Show resolved Hide resolved
test/unit/utils_test.js Outdated Show resolved Hide resolved
test/unit/utils_test.js Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
test/unit/utils_test.js Outdated Show resolved Hide resolved
leighmcculloch
leighmcculloch reviewed Nov 5, 2020
View reviewed changes
Copy link
Member

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

Thanks for turning this around quickly 👏 . I left a few comments below. Some are ideas/suggestions that might be beneficial (💡 ) but not critical or required. Two asks (❗ ). I skipped over the tests because the formatting changes were pretty noisy and I'd like to take another look after the formatting changes are moved to a separate PR, if that can happen.

src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
test/unit/utils_test.js Outdated Show resolved Hide resolved
quietbits and others added 3 commits November 6, 2020 09:21
@quietbits
Undo Prettier formatting
859ec3c
@quietbits @JakeUrban @leighmcculloch
Update descriptions
e069106 
Co-authored-by: Jake Urban <10968980+JakeUrban@users.noreply.github.com>
Co-authored-by: Leigh McCulloch <leigh@stellar.org>
@quietbits
Small refactoring + update home domain tests
b2f62a9
JakeUrban
JakeUrban approved these changes Nov 6, 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.

Looks good to me pending the non-optional homeDomain parameter change.

leighmcculloch
leighmcculloch approved these changes Nov 6, 2020
View reviewed changes
Copy link
Member

@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.

🎉 This looks great. Thank you!

Could you also add a changelog entry for this. Take a look at this previous change for how to do that:
de76a0d#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR7-R12

@leighmcculloch
Copy link
Member

leighmcculloch commented Nov 6, 2020 •
edited
Loading

In terms of merging this change we need to wait to merge it until stellar/stellar-protocol#746 merges, and then merge them as the same time. When this merges, we'll need to open a separate PR similar to #585 except this is a breaking change so we'll need to bump the major version. cc @abuiles

JakeUrban
JakeUrban approved these changes Nov 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.

Just one suggestion, otherwise it looks good!

src/utils.ts Outdated Show resolved Hide resolved
leighmcculloch
leighmcculloch approved these changes Nov 9, 2020
View reviewed changes
Copy link
Member

@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.

This looks great. I left a couple suggestions (💡 ) below, but regardless this is great. Thank you!

CHANGELOG.md Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
quietbits and others added 4 commits November 9, 2020 13:30
@quietbits @leighmcculloch
Update src/utils.ts
b08cd26 
Co-authored-by: Leigh McCulloch <leigh@stellar.org>
@quietbits @leighmcculloch
Update CHANGELOG.md
57b6ca4 
Co-authored-by: Leigh McCulloch <leigh@stellar.org>
@quietbits @leighmcculloch
Improve homeDomains param description
80d5b64 
Co-authored-by: Leigh McCulloch <leigh@stellar.org>
@quietbits
Fix test
934dd11
@quietbits quietbits merged commit c0728b9 into master Nov 9, 2020
@quietbits quietbits deleted the il-home-domain-check branch November 9, 2020 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leighmcculloch leighmcculloch leighmcculloch approved these changes

@JakeUrban JakeUrban JakeUrban approved these changes

@morleyzhi morleyzhi morleyzhi approved these changes

Assignees
No one assigned
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)
4 participants
@quietbits @leighmcculloch @morleyzhi @JakeUrban