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.

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

SEP-23: Propose {ed25519}{id} for look-a-like G and M addresses #895

Merged
merged 5 commits into from
Apr 9, 2021

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented Mar 23, 2021

What

Make G and M addresses look more a-like, by making M addresses so that they contain most of the G address:

Example of what it looks like without this change:
GAQAA5L65LSYH7CQ3VTJ7F3HHLGCL3DSLAR2Y47263D56MNNGHSQSTVY + 1234 =
MAAAAAAAAAAAJURAAB2X52XFQP6FBXLGT6LWOOWMEXWHEWBDVRZ7V5WH34Y22MPFBHUHY

Example of what it will look like with this change:
GAQAA5L65LSYH7CQ3VTJ7F3HHLGCL3DSLAR2Y47263D56MNNGHSQSTVY + 1234 =
MAQAA5L65LSYH7CQ3VTJ7F3HHLGCL3DSLAR2Y47263D56MNNGHSQSAAAAAAAAAAE2LP26

Why

When experimenting with SEP-23 and M addresses I found myself mixing up which M addresses were associated with which G address because there was nothing similar looking about them, even though most of the M address is the same data within the G address.

I think for developer friendliness it would be helpful if we make M addresses look like they contain approximately the same data. Doing this won't compromise the opaqueness that we want M addresses to have for end-users, where opaqueness matters most, but for developers or account operators who are handling a G address and its corresponding M addresses it is much easier to see that an M address is a muxed identifier for a specific G address.

The changes required for this in any implementation are really simple. Instead of encoding the Id then the Ed25519 value, we encode the Ed25519 value then the Id.

This change has no bearing on the underlying XDR or protocol.

ire-and-curses
ire-and-curses previously approved these changes Mar 23, 2021
Copy link
Member

@ire-and-curses ire-and-curses left a comment

Choose a reason for hiding this comment

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

Yeah this looks great to me. I don't see any downside and the upside is clear.

@leighmcculloch
Copy link
Member Author

I still need to update the failure test cases before merging this.

@2opremio
Copy link
Contributor

2opremio commented Apr 5, 2021

@leighmcculloch any timeline for this? Ideally, I would like it to be merged before I finish implementing stellar/go#3490

@2opremio
Copy link
Contributor

2opremio commented Apr 8, 2021

I updated the invalid test cases and rebased, it should be good to go.

@2opremio
Copy link
Contributor

2opremio commented Apr 8, 2021

BTW, the test cases I added have been checked at stellar/go#3527

@leighmcculloch
Copy link
Member Author

Thanks @2opremio! @stanford-scs this is ready for your final review.

Copy link
Contributor

@stanford-scs stanford-scs left a comment

Choose a reason for hiding this comment

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

I'm okay merging this, but it would be nice if the order were switched in bullet point 4. Since I don't know how to submit a pull request on a pull request, I'll just leave this feedback here.

@2opremio
Copy link
Contributor

2opremio commented Apr 9, 2021

@stanford-scs thanks. I am not sure about the order of which you are referring to exactly, so I think I will merge and let you create a separate PR for that if that's OK.

@yawebs

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants