Improve payloadId generator

[everdimension]

Problem

Current payloadId function has defects:

• It doesn't guarantee to return a unique value (due to Math.random() not being suited for cryptography and due to single-threadedness of javascript)
• It doesn't guarantee to always return a value greater than the previous one (since Date.now() can return the same value if called in the same tick)

Solution 1 Just use randomUUID(). This breaks the expected return type of current payloadId which is a number

Solution 2 Preserves function signature The most simple, performant and (in my opinion) sufficient solution would be the following:

export function getPayloadId(): number {
  return crypto.getRandomValues(new Uint16Array(1))[0];
}

This would provide a sufficiently random number.

Solution 3 Provide incremental values To always return a random value greater than the previous one, we have to introduce some state:

let base = 1;

export function getPayloadId(): number {
  return (base++ << 16) + crypto.getRandomValues(new Uint16Array(1))[0];
}

This way, getPayloadId() will guarantee a random value to be always greater than the previous one, even if the function is called in the same tick. But this guarantee would only exist within a single runtime, so it will not be preserved between different scripts or session restarts.

Solution 4 This is the solution from the current PR. It's quite convoluted, but aims to prevent breaking changes.

export function payloadId(entropy = 3): number {
  const generator = entropy > 3 ? uint16Generator : uint8Generator;
  const shift = entropy > 3 ? 1000000 : 10000;
  const date = Date.now() * Math.pow(10, shift);
  const extra = generator.getRandomValue();
  return date + extra;
}

This guarantees incremental nature of the function if called within a single runtime and has a quite small intersection window of values between script restarts. Relying on timestamp can't guarantee us anything anyway because they can differ between systems, so this intersection I think is ok. Also, this solution kinda ignores the entropy parameter (not fully, just branching whether or not it's larger than 3), but I don't see much value in preserving it.

---

Tell me what you think. My personal vote is for Solution 2, but I understand that it might be undesired to get rid of the incremental nature of the function, even if it worked improperly.

[everdimension]

@arein Well, since this is a unique-number generator you can't conventionally unit-test it. The current payloadId implementation doesn't have tests either.

Possible tests I can add are:

• Test that function returns a number
• Test that for several outputs each one is greater than the previous one and they're unique (though it needs to be understood that this doesn't test true uniqueness)
• Test that for several outputs the values are within the expected bounds.

Is that what you had in mind?

[arein]

@arein Well, since this is a unique-number generator you can't conventionally unit-test it. The current payloadId implementation doesn't have tests either.

Possible tests I can add are:

• Test that function returns a number
• Test that for several outputs each one is greater than the previous one and they're unique (though it needs to be understood that this doesn't test true uniqueness)
• Test that for several outputs the values are within the expected bounds.

Is that what you had in mind?

Yes that sounds good. It's obviously really sensitive and crucial logic and many of our systems e.g. the deduplication mechanism in the SDKs itself depend on properties of the generated ids such as uniqueness.

The current payloadId implementation doesn't have tests either

Thanks for raising the bar here

[everdimension]

@arein Yeah, writing tests for these was a good idea

• I added tests which are actually failing tests for the current payloadId() implementation

• I updated the random generator. Now that we use local state, it makes no sense to generate a new random number on each call and it only complicates the code.

• I also removed entropy parameter from payloadId. It was buggy anyway, since passing any value larger than 3 would produce a number larger than Number.MAX_SAFE_INTEGER, which in turn would increase the likeliness of repeated numbers even for different timestamps.

[ganchoradkov]

Thank you for the PR @everdimension 💯 💯
The new ID generation looks solid and having the tests to back that up is awesome! I will spend some time testing it out to confirm there wouldn't be any surprises and backwards compatibility is preserved 💯

[everdimension]

@ganchoradkov Did you have time to test this? Is there anything I can help with?

[ganchoradkov]

@everdimension, I'm on it today 👍

[everdimension]

thank you for the PR @everdimension, I tested with

• 1.0.8-canary.0 - 23 digits IDs -> doesn't work
• 1.0.8-canary.1 - 19 digits IDs that I suggested, works correctly

What do you mean by "doesn't work" btw? So some code relies on the generated id length? In which codebase?

[ganchoradkov]

@everdimension https://github.com/WalletConnect/walletconnect-monorepo, it contains the main JS clients such as web3wallet

[everdimension]

@ganchoradkov Pushed the update: https://github.com/WalletConnect/walletconnect-utils/pull/168/commits/cbaedce5b8acdeda8db8f701b16225eb024c8ce6