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

refactor!: remove uuid and rand dependencies where applicable #7939

Merged
merged 12 commits into from
Oct 17, 2023

Conversation

amrbashir
Copy link
Member

ref: #7756

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@amrbashir amrbashir requested a review from a team as a code owner October 2, 2023 15:27
@amrbashir
Copy link
Member Author

amrbashir commented Oct 2, 2023

There is a couple of places that still use rnd and uuid crates:

  1. isolation pattern still uses uuid, not sure if we can replace it with a simple counter, cc @chippers
  2. csp nonce and asset protocol range requests -> migrated to the lighter getrandom crate in d398300

@amrbashir amrbashir changed the title refactor: remove uuid and rand dependencies where applicable refactor!: remove uuid and rand dependencies where applicable Oct 2, 2023
@chippers
Copy link
Member

chippers commented Oct 3, 2023

There is a couple of places that still use rnd and uuid crates:

  1. isolation pattern still uses uuid, not sure if we can replace it with a simple counter, cc @chippers
  2. csp nonce and asset protocol range requests -> migrated to the lighter getrandom crate in d398300

it's used so that it's unpredictable and because the output chars are always valid as a schema suffix. using getrandom to generate 128 bits and using lowercase hex as the output should have the same effect here.

i've noticed some code that used uuid is replaced with smaller sizes of things, so take care that it's not problematic there. uuid v4 are often used as trusted random unique numbers because they are 128 bytes (well 122 since some are fixed) that you don't need to check for collisions for in most applications because it's so incredibly unlikely to get another of the same. u32s are much more likely to collide

some places they have been replaced with constants (like the event listener object and function names) which was seen as undesirable before to discourage people from using it manually and going through the proper api instead.

@amrbashir
Copy link
Member Author

amrbashir commented Oct 3, 2023

it's used so that it's unpredictable and because the output chars are always valid as a schema suffix. using getrandom to generate 128 bits and using lowercase hex as the output should have the same effect here

I will leave that one using uuid for now, at least it is only active when using isolation pattern and the main goal for this PR was to remove it from public API.

i've noticed some code that used uuid is replaced with smaller sizes of things, so take care that it's not problematic there. uuid v4 are often used as trusted random unique numbers because they are 128 bytes (well 122 since some are fixed) that you don't need to check for collisions for in most applications because it's so incredibly unlikely to get another of the same. u32s are much more likely to collide

the u32s are only incrementing so it shouldn't collide with itself, there maybe other ids that could have the same value but they are used in other places.

some places they have been replaced with constants (like the event listener object and function names) which was seen as undesirable before to discourage people from using it manually and going through the proper api instead.

Yeah, I wasn't aware of the history behind that, I could use getrandom in this case but may I ask why it was discouraged? I mean we don't ever provide support for those who uses it directly and we can break it without any notice.

@chippers
Copy link
Member

Yeah, I wasn't aware of the history behind that, I could use getrandom in this case but may I ask why it was discouraged? I mean we don't ever provide support for those who uses it directly and we can break it without any notice.

just to avoid the typical "spacebar heating" problem if the behavior does change - and we should explicitly mark somewhere that it is unstable

@amrbashir
Copy link
Member Author

could change the const from

function_name: "_listeners_function_id_",
object_name: "_listeners_object_id_",

to

function_name: "_internal_listeners_function_id_",
object_name: "_internal_listeners_object_id_",

lucasfernog
lucasfernog previously approved these changes Oct 16, 2023
@amrbashir amrbashir merged commit 2558fab into dev Oct 17, 2023
2 checks passed
@amrbashir amrbashir deleted the refactor/remove-unneded-rand-uuid-usages branch October 17, 2023 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔎 In audit
Development

Successfully merging this pull request may close these issues.

3 participants