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.

Sign up for GitHub

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

Jump to bottom

fix: use deterministic writeable account order #21724

Merged
merged 1 commit into from
Jan 8, 2022
Merged

fix: use deterministic writeable account order #21724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion web3.js/src/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link

@mvayngrib mvayngrib Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

can i ask why localeCompare? These are public keys, not sure what internationalization has to do with it 🤔

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

@mvayngrib mvayngrib Aug 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

it breaks in react native depending on which JS engine u use, that's how we noticed it. Might be worth refactoring

Copy link
Member

Choose a reason for hiding this comment

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

Oh ok, let's refactor then. Do you have the bandwidth to PR the change?

Copy link

Choose a reason for hiding this comment

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

@jstarry i'll ask someone on my team to upstream the fix 👍

Copy link
Member

Choose a reason for hiding this comment

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

Excellent, thanks!

Copy link

Choose a reason for hiding this comment

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

@jstarry @mvayngrib apologizes for the delay, but I just created a PR to upstream the fix.

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 ? pubkeySorting : x.isWritable ? -1 : 1;
return checkSigner || checkWritable;
});

Expand Down
84 changes: 83 additions & 1 deletion web3.js/test/transaction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {expect} from 'chai';

import {Keypair} from '../src/keypair';
import {PublicKey} from '../src/publickey';
import {Transaction} from '../src/transaction';
import {Transaction, TransactionInstruction} from '../src/transaction';
import {StakeProgram} from '../src/stake-program';
import {SystemProgram} from '../src/system-program';
import {Message} from '../src/message';
Expand Down Expand Up @@ -505,4 +505,86 @@ describe('Transaction', () => {
tx.addSignature(from.publicKey, toBuffer(signature));
expect(tx.verifySignatures()).to.be.true;
});

it('can serialize, deserialize, and reserialize with a partial signer', () => {
const signer = Keypair.generate();
const acc0Writable = Keypair.generate();
const acc1Writable = Keypair.generate();
const acc2Writable = Keypair.generate();
const t0 = new Transaction({
recentBlockhash: 'HZaTsZuhN1aaz9WuuimCFMyH7wJ5xiyMUHFCnZSMyguH',
feePayer: signer.publicKey,
});
t0.add(
new TransactionInstruction({
keys: [
{
pubkey: signer.publicKey,
isWritable: true,
isSigner: true,
},
{
pubkey: acc0Writable.publicKey,
isWritable: true,
isSigner: false,
},
],
programId: Keypair.generate().publicKey,
}),
);
t0.add(
new TransactionInstruction({
keys: [
{
pubkey: acc1Writable.publicKey,
isWritable: false,
isSigner: false,
},
],
programId: Keypair.generate().publicKey,
}),
);
t0.add(
new TransactionInstruction({
keys: [
{
pubkey: acc2Writable.publicKey,
isWritable: true,
isSigner: false,
},
],
programId: Keypair.generate().publicKey,
}),
);
t0.add(
new TransactionInstruction({
keys: [
{
pubkey: signer.publicKey,
isWritable: true,
isSigner: true,
},
{
pubkey: acc0Writable.publicKey,
isWritable: false,
isSigner: false,
},
{
pubkey: acc2Writable.publicKey,
isWritable: false,
isSigner: false,
},
{
pubkey: acc1Writable.publicKey,
isWritable: true,
isSigner: false,
},
],
programId: Keypair.generate().publicKey,
}),
);
t0.partialSign(signer);
const t1 = Transaction.from(t0.serialize());
t1.serialize();
});
});