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

Multiplexed AccountIDs SEP+CAP #405

Merged
merged 7 commits into from
Dec 17, 2019
Merged

Multiplexed AccountIDs SEP+CAP #405

merged 7 commits into from
Dec 17, 2019

Conversation

stanford-scs
Copy link
Contributor

No description provided.


1. Start with the appropriate version byte from the above table.

2. If it is a multiplexed address, append an 8-byte memo ID in
Copy link
Member

Choose a reason for hiding this comment

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

This proposal limits services to 8 byte memo IDs and not memos containing text. What's the motivation for that? Is it based on common usage in the ecosystem? A more flexible memo supporting other memo types might be easier to integrate with for services that identify transfers with alphanumeric identifiers and not integers.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Most exchanges indeed use MEMO_ID (uint64), though they tolerate getting a MEMO_STRING that contains a decimal representation of that id.
  • MEMO_STRING are intended for human entered strings so they don't really fit this use case.
  • I believe that 2 bytes (2^64 permutations) is a large enough space and I can't think of a scenario in which it's not enough. 32 byte memos were intended as a means to encode a return address in a memo. It's (a) not really used and (b) doesn't fit this use cases.
  • We can make this size variable but then keys could have different sizes which is a bit weird.
  • MEMO_STRING and MEMO_HASH can still be used, just not encoded in the account id.

Copy link
Member

Choose a reason for hiding this comment

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

The 8 byte limit on unique identifiers might be what exchanges are comfortable with today, but IDs with larger spaces are more commonly used in software systems of scale, e.g. UUID (16 bytes), MongoID (12 bytes). It will make integration more straight forward for existing large systems if it's possible to reuse the same identifiers because in systems of scale expecting to generate IDs in a smaller space reliably can be challenging and require coordination.

Good point about MEMO_STRING not really being suited for this case, as it's too small at 28 bytes to encode a 16 byte ID as a hex string which would require 32 bytes.

We can make this size variable but then keys could have different sizes which is a bit weird.

Strkeys can already be of different lengths?

Copy link
Contributor

@bartekn bartekn Nov 21, 2019

Choose a reason for hiding this comment

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

Strkeys can already be of different lengths?

Current strkeys are always 32 bytes long after decoding. M keys will be longer but I think is fine because it's a breaking change anyway.


~~~ {.c}
struct MultiplexedAccountID {
uint64 *memoID;
Copy link
Member

@leighmcculloch leighmcculloch Sep 17, 2019

Choose a reason for hiding this comment

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

What's the motivation for placing the optional field at the beginning of the struct? Conceptually it may be more intuitive to place required fields first, optional fields second.

| STRKEY\_HASH\_X | 23 << 3 | X | no |


The following steps transform a binary key into a strkey:
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with this algorithm is that base32 symbols are 5 bit and (1+8+32+2)*8 % 5 == 3. This may require devs to update existing strkey encoders/decoders code if it doesn't support padding. Maybe it's worth adding 2 extra null bytes after a version byte? It will increase strkey encoded address by 3 chars though.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's worst than that: when the payload is not a multiple of 5 bits, libraries have to implement a version without padding but because the RFC discourages doing that, not using padding bytes is likely not supported/buggy.
For example, I found multiple bugs in just the base32 encoders/decoders that we use in core when non aligned payloads were involved (originally, I was not using 8 bits for the version when designing "StrKey" and ran into this issue).

So if we pad the payload with 2 extra bytes, we add a total of 10 bytes of data, 56+16=72 characters which is not that different from the version that relies on proper handling of non padded non aligned.

Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds like a valid concern.

What if we change the Multiplexed Version byte 0x60 (12 << 3) to be multiple bytes 0x65 0x17 0x3f?

This adds the two bytes required to avoid padding and introduces a prefix of MULT instead of just M.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First off, if you are padding, RFC4648 requires = signs if the input is not a multiple of 5 bytes, not 5 bits. However, for crypto agility we are going to need the ability to have arbitrary length public keys. That means any padding should be in the Strkey specification, not the underlying XDR, as otherwise we'll keep having this problem.

I personally think that we should just require the variant without padding. This stuff isn't that hard to implement and people shouldn't be relying on third-party libraries anyway, unless those libraries are highly trusted (like built into your language runtime). Someone who controls these padding libraries can steal funds.

That said, if we really want to pad, then we should pad with = as specified in the RFC. We should definitely not invent a new padding, and even less violate layering and start placing restrictions on XDR key formats just so that the can be base-32 encoded a certain way.

@stanford-scs
Copy link
Contributor Author

I've updated this to include both a SEP and a CAP version.

@tomerweller tomerweller changed the title Initial draft of SEP-0023 for multiplexed AccountIDs that require the use of memos Multiplexed AccountIDs SEP+CAP Dec 5, 2019
@tomerweller tomerweller merged commit 2be91ce into stellar:master Dec 17, 2019
@tomerweller
Copy link
Contributor

I've merged this to make it easier to refer to these proposals. They are still in draft though.

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