fix: use deterministic writeable account order #21724
Conversation
web3.js/src/transaction.ts
Outdated
| @@ -261,7 +261,7 @@ export class Transaction { | |||
| accountMetas.sort(function (x, y) { | |||
| const checkSigner = x.isSigner === y.isSigner ? 0 : x.isSigner ? -1 : 1; | |||
| const checkWritable = | |||
| x.isWritable === y.isWritable ? 0 : x.isWritable ? -1 : 1; | |||
| x.isWritable === y.isWritable ? x.pubkey.toBuffer().compare(y.pubkey.toBuffer()) : x.isWritable ? -1 : 1; | |||
There was a problem hiding this comment.
We also need this ordering for checkSigner right?
There was a problem hiding this comment.
I don't think so. Just spent a bit trying it out.
The checkSigner || checkWritable means that it falls back to the writable ordering. So writable signers go earlier. If both signers aren't writable, then it uses the fallback buffer sorting.
|
Looks good, do you mind fixing the lint issues? |
ChewingGlass
force-pushed
the
master
branch
2 times, most recently
from
4d182bd
to
06b473f
Compare
Should be good |
|
@jstarry it says first-time contributors need a maintainer to approve running workflows. Lmk if there's anything else I can do on this one |
thanks for the ping, just kicked off the job. |
jstarry
changed the title
Needs another workflow run, fixed the issue I think. |
Codecov Report
@@ Coverage Diff @@
## master #21724 +/- ##
===========================================
- Coverage 81.6% 70.1% -11.6%
===========================================
Files 511 35 -476
Lines 143320 2084 -141236
Branches 0 299 +299
===========================================
- Hits 116976 1461 -115515
+ Misses 26344 521 -25823
- Partials 0 102 +102 |
|
thx for your patience! |
jstarry
changed the title
| @@ -259,9 +259,12 @@ export class Transaction { | |||
|
|
|||
| // Sort. Prioritizing first by signer, then by writable | |||
| accountMetas.sort(function (x, y) { | |||
| const pubkeySorting = x.pubkey | |||
| .toBase58() | |||
| .localeCompare(y.pubkey.toBase58()); | |||
There was a problem hiding this comment.
can i ask why localeCompare? These are public keys, not sure what internationalization has to do with it 🤔
There was a problem hiding this comment.
I didn't notice this in review but agreed that it's overkill here, direct comparison should be fine here. It's fine as is though, imo
There was a problem hiding this comment.
it breaks in react native depending on which JS engine u use, that's how we noticed it. Might be worth refactoring
There was a problem hiding this comment.
Oh ok, let's refactor then. Do you have the bandwidth to PR the change?
There was a problem hiding this comment.
@jstarry i'll ask someone on my team to upstream the fix 👍
There was a problem hiding this comment.
@jstarry @mvayngrib apologizes for the delay, but I just created a PR to upstream the fix.
Problem
See #21722
Summary of Changes
See #21722
Fixes #
Fixes ordering of writeable accounts to always be deterministic.