Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

refactor(experimental): allow the nonce authority account to be writable #1777

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

mcintyre94
Copy link
Contributor

@mcintyre94 mcintyre94 commented Oct 24, 2023

This PR updates the logic to determine whether a given instruction is a durable nonce transaction. Specifically, the nonce authority address can now be writeable. This is the case if the nonce authority is also the fee payer, for example in a transaction created with the CLI: https://explorer.solana.com/tx/2FZPN5ZJqi5LSMe18auxx59G1VZAP71p8KLCDvyH1WAFgqEXfag6xmC8KnzXQW3KSdbmEyWp8rdd1VnmxMsTrUdG?cluster=devnet

I don't think any change is required for other accounts

  • The nonce address is unlikely to need to be a signer, since it is already initialized
  • I think sysvar addresses are always readonly

Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Lgtm mate!

Copy link
Contributor

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

This is the case if the nonce authority is also the fee payer, for example in a transaction created with the CLI:

Hang on though; the way that transactions are compiled in the new web3.js is such that the AccountRole on any individual instruction has no bearing alone on what the AccountRole will be at the level of the entire transaction.

If the advance nonce instruction specifies account 'abc' as READONLY_SIGNER and the transaction fee payer is set to account 'abc', the transaction compiler will combine all of the AccountRoles of 'abc' in the transaction and compute the highest role (ie. WRITABLE_SIGNER in that case).

export function mergeRoles(roleA: AccountRole.WRITABLE, roleB: AccountRole.READONLY_SIGNER): AccountRole.WRITABLE_SIGNER; // prettier-ignore
export function mergeRoles(roleA: AccountRole.READONLY_SIGNER, roleB: AccountRole.WRITABLE): AccountRole.WRITABLE_SIGNER; // prettier-ignore
export function mergeRoles(roleA: AccountRole, roleB: AccountRole.WRITABLE_SIGNER): AccountRole.WRITABLE_SIGNER; // prettier-ignore
export function mergeRoles(roleA: AccountRole.WRITABLE_SIGNER, roleB: AccountRole): AccountRole.WRITABLE_SIGNER; // prettier-ignore
export function mergeRoles(roleA: AccountRole, roleB: AccountRole.READONLY_SIGNER): AccountRole.READONLY_SIGNER; // prettier-ignore
export function mergeRoles(roleA: AccountRole.READONLY_SIGNER, roleB: AccountRole): AccountRole.READONLY_SIGNER; // prettier-ignore
export function mergeRoles(roleA: AccountRole, roleB: AccountRole.WRITABLE): AccountRole.WRITABLE; // prettier-ignore
export function mergeRoles(roleA: AccountRole.WRITABLE, roleB: AccountRole): AccountRole.WRITABLE; // prettier-ignore
export function mergeRoles(roleA: AccountRole.READONLY, roleB: AccountRole.READONLY): AccountRole.READONLY; // prettier-ignore
export function mergeRoles(roleA: AccountRole, roleB: AccountRole): AccountRole; // prettier-ignore
export function mergeRoles(roleA: AccountRole, roleB: AccountRole): AccountRole {
return roleA | roleB;
}

It should be fine, therefore, to type the AdvanceNonce instruction with the lowest AccountRole necessary to advance the nonce, and let the transaction compiler decide the ultimate role for every account at the transaction level.

Comment on lines 104 to 105
(instruction.accounts[2].role === AccountRole.READONLY_SIGNER ||
instruction.accounts[2].role === AccountRole.WRITABLE_SIGNER)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use isSignerRole() here if you feel like it.

@@ -129,6 +129,29 @@ describe('assertIsDurableNonceTransaction()', () => {
assertIsDurableNonceTransaction({ ...durableNonceTx });
}).not.toThrow();
});
it('does not throw when the nonce authority is a writeable signer', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('does not throw when the nonce authority is a writeable signer', () => {
it('does not throw when the nonce authority is a writable signer', () => {

Copy link
Contributor Author

@mcintyre94 mcintyre94 Oct 25, 2023

Choose a reason for hiding this comment

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

I don't even have the excuse of saying that's a British spelling!

Base automatically changed from add-tx-deserialize to master October 25, 2023 09:02
- this is required because the nonce authority can be writable, eg as
the transaction fee payer
@mcintyre94 mcintyre94 force-pushed the nonce-authority-writable branch from dc76c9d to 773645a Compare October 25, 2023 09:07
@mcintyre94
Copy link
Contributor Author

Discussed on Slack, but just so it's here too - I forgot to mention that this is related to decompiling a transaction.

In a compiled transaction the account has had its roles merged, and when we apply the merged role to the instruction it may be different to what it originally had in that instruction (eg in this case the read-only signer is now a writable signer because of something else making it writable)

So basically it's something else that's lossy when we compile. We could special case the instruction on decompile but I think that would lead to more complexity.

@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.87.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants