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(experimental): add a function to assert a transaction has a blockhash lifetime #1908

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

mcintyre94
Copy link
Contributor

This PR adds a new function assertIsBlockhashLifetimeTransaction which narrows the type of a transaction to BaseTransaction & ITransactionWithBlockhashLifetime, if the transaction contains a valid blockhash lifetime constraint

It mirrors the behaviour of the existing assertIsDurableNonceTransaction

My specific use case for this is using the default transaction confirmer with a deserialized transaction. The default transaction confirmer requires the lastValidBlockHeight field of the lifetime constraint:

transaction: ITransactionWithFeePayer &
ITransactionWithSignatures &
Readonly<{
lifetimeConstraint: {
lastValidBlockHeight: Slot;
};
}>;
}

By asserting that the deserialized transaction has a valid blockhash lifetime constraint, we can satisfy this requirement and that of any other code that requires a blockhash lifetime on the transaction.

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, but for consistency I think this should be called assertIsTransactionWithBlockhashLifetime. Wdyt?

Comment on lines 47 to 70
function isBlockhashLifetimeTransaction(
transaction: BaseTransaction | (BaseTransaction & ITransactionWithBlockhashLifetime)
): transaction is BaseTransaction & ITransactionWithBlockhashLifetime {
return (
'lifetimeConstraint' in transaction &&
typeof transaction.lifetimeConstraint.blockhash === 'string' &&
typeof transaction.lifetimeConstraint.lastValidBlockHeight === 'bigint'
);
}

export function assertIsBlockhashLifetimeTransaction(
transaction: BaseTransaction | (BaseTransaction & ITransactionWithBlockhashLifetime)
): asserts transaction is BaseTransaction & ITransactionWithBlockhashLifetime {
if (!isBlockhashLifetimeTransaction(transaction)) {
// TODO: Coded error.
throw new Error('Transaction is not a blockhash transaction');
}
assertIsBlockhash(transaction.lifetimeConstraint.blockhash);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it's on purpose but the logic of the isX and assertIsX functions is not the same. Passing a transaction with an invalid base58 blockhash will work for isX but fail for assertIsIx.

To make them the same, we could push that conditional to the assertIsX function and just do a try/catch on the isX function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thanks! I've refactored to move all the logic to the isX function, which makes them symmetrical as you say and also mirrors the durable nonce equivalent.

@mcintyre94
Copy link
Contributor Author

Lgtm, but for consistency I think this should be called assertIsTransactionWithBlockhashLifetime. Wdyt?

Agreed, renamed to that!

): transaction is BaseTransaction & ITransactionWithBlockhashLifetime {
const lifetimeConstraintShapeMatches =
'lifetimeConstraint' in transaction &&
typeof transaction.lifetimeConstraint.blockhash === 'string' &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use assertIsBlockhash here, but meh.

Copy link
Collaborator

@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.

giphy.gif

@mcintyre94 mcintyre94 merged commit ae94ca3 into master Nov 30, 2023
7 checks passed
@mcintyre94 mcintyre94 deleted the assert-blockhash-tx branch November 30, 2023 10:49
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 Dec 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants