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

SEP-45: Replay prevention + argument name fixes #1639

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

SEP-45: Replay prevention + argument name fixes #1639

wants to merge 1 commit into from

Conversation

philipliu
Copy link
Contributor

@philipliu philipliu commented Jan 30, 2025 •
edited
Loading

  • Rename Home Domain Address to Server Account
  • Rename Client Domain Address to Client Domain Account
  • Rename home_domain_address argument to web_auth_domain_signer
  • Rename client_domain_address argument to client_domain_signer
  • Add home_domain argument to web_auth_verify function
  • Mark nonce verification as required and add a replay prevention section
  • Clarify client domain signing sentence

@philipliu philipliu marked this pull request as ready for review January 30, 2025 18:12
@marcelosalloum
Copy link
Contributor

I found an occurrence of Home Domain Address here:
https://github.com/philipliu/stellar-protocol/blob/6abbb17c6ec833eb282b11a2a656039b08e113ab/ecosystem/sep-0045.md?plain=1#L60-L61

marcelosalloum
marcelosalloum reviewed Feb 8, 2025
View reviewed changes
ecosystem/sep-0045.md
Address** and contains a valid signature from the **Home Domain Address**
1. The `client_domain_signer` value matches the **Client Domain Account**
1. The **Client** verifies that there is an authorization entry where `credentials.address.address` is the **Server
Account** and contains a valid signature from the **Server Account**
1. The **Client** signs the authorization entry where `credentials.address.address` is the **Client Account** using the
secret key(s) of the signer(s) for the **Client Account**
- Note: **Client** signatures may not be required if the contract's `__check_auth` implementation does not require
Copy link
Contributor

@marcelosalloum marcelosalloum Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Can we make it clear that this contract refers to the Client's contract account? When I read it I assumed this was referring to the SEP-45 contract.

Suggested change
- Note: **Client** signatures may not be required if the contract's `__check_auth` implementation does not require
- Note: **Client** signatures may not be required if the Client contract's `__check_auth` implementation does not require

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcelosalloum marcelosalloum marcelosalloum left review comments

@leighmcculloch leighmcculloch Awaiting requested review from leighmcculloch

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.
2 participants
@philipliu @marcelosalloum