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

fix: remove localeCompare from account order sorting #28234

Closed
wants to merge 1 commit into from

Conversation

cassapula
Copy link

@cassapula cassapula commented Oct 5, 2022

Problem

localeCompare should be limited and not used when not required. For the sorting of the account metas, we don't specifically require the usage of it since those are Public Keys, and a direct comparison would work as expected.

This is prone to issues in react native, depending on the JS engine being used.

Context: Conversation on the #21724.

Summary of Changes

This fix makes the sorting of account metas work predictably independent of the JS engine used, as localeCompare behaves differently depending on the JS engine (v8 vs JSC, for example).

@mergify mergify bot added the community Community contribution label Oct 5, 2022
@mergify mergify bot requested a review from a team October 5, 2022 15:56
Copy link
Contributor

@jordaaash jordaaash left a comment

Choose a reason for hiding this comment

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

Not using localCompare maybe makes sense, but I think the implementation here has issues.

// Otherwise, sort by pubkey, stringwise.
return x.pubkey.toBase58().localeCompare(y.pubkey.toBase58());

const xPubKey = x.pubkey.toBase58().toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why lowercase? This will change the sort order. It will also cause two pubkeys which are not identical to be interpreted as identical. Does that matter?

const xPubKey = x.pubkey.toBase58().toLowerCase();
const yPubKey = y.pubkey.toBase58().toLowerCase();

return xPubKey < yPubKey ? -1 : xPubKey > yPubKey ? 1 : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Using < and > doesn't seem right. The string may start with a number, causing it to be parsed as a number and then compared with another string (that may not start with a number).

From https://www.w3schools.com/js/js_comparisons.asp

When comparing a string with a number, JavaScript will convert the string to a number when doing the comparison. An empty string converts to 0. A non-numeric string converts to NaN which is always false.

Is this desired behavior?

@steveluscher steveluscher changed the title fix: remove localCompare from account order sorting fix: remove localeCompare from account order sorting Oct 5, 2022
@steveluscher
Copy link
Contributor

I remember bringing this up when we made the last change to this code as seeming off (what locale could possibly affect the order materially) but we never ended up pulling on the thread.

@steveluscher
Copy link
Contributor

Also, I'm curious to know the details of the different behavior on RN. What engine? What strings produce different orders?

@steveluscher
Copy link
Contributor

steveluscher commented Oct 5, 2022

To fix the RN mismatch: did we consider figuring out what the difference was and being explicit in our call to localeCompare (ie. supplying both a hard-coded locale, and a static set of Intl.Collator options)

@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 29, 2022
@github-actions github-actions bot closed this Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution stale [bot only] Added to stale content; results in auto-close after a week.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants