Allow messages with no lifetime to be compiled #581
Conversation
🦋 Changeset detectedLatest commit: ccad1a3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BundleMonFiles updated (7)
Unchanged files (120)
Total files change +273B +0.08% Final result: ✅ View report in BundleMon website ➡️ |
ef66fd3 to
01ec4de
Compare
|
Documentation Preview: https://kit-docs-kkznyu7do-anza-tech.vercel.app |
f9a39f0 to
01380d2
Compare
4d50941 to
e943aea
Compare
| @@ -216,7 +216,7 @@ export type DecompileTransactionMessageConfig = { | |||
| export function decompileTransactionMessage( | |||
| compiledTransactionMessage: CompiledTransactionMessage, | |||
| config?: DecompileTransactionMessageConfig, | |||
| ): CompilableTransactionMessage { | |||
| ): BaseTransactionMessage & TransactionMessageWithFeePayer & TransactionMessageWithLifetime { | |||
There was a problem hiding this comment.
I'm not sure about returning TransactionMessageWithLifetime here. My concern is that if you compile a message without a lifetime, and then decompile it, should we say it now has a lifetime?
Maybe, it's going to have an all-zero blockhash in the lifetimeConstraint field. But I wonder if it'd be better to make apps narrow to TransactionMessageWithLifetime themselves after doing their own checks.
There was a problem hiding this comment.
Yeah this is the part where I don't think either solution is ideal.
The reality is, whenever we decompile, we always fill in the lifetimeConstraint property of the transaction message, even if it's all zeros. So structurally we do always return a TransactionMessageWithLifetime.
However, you are right that it feels weird that compiling/decompiling a message magically introduces the TransactionMessageWithLifetime flag.
But, decompiling a message will typically be used to find information about a transaction that landed and therefore it would be a shame to make apps always use isTransactionMessageWithBlockhashLifetime and isTransactionMessageWithDurableNonceLifetime when the data is technically always there.
My interpretation of this is kinda like the From and To type parameters of Codecs. From is allowed to be looser than To because the encoder can fill some gaps. However, decoding is stricter because we cannot fill any gaps if information is missing from the provided bytes.
I'm happy with either solution though.
There was a problem hiding this comment.
I think you're probably right. There are use cases like wallets and relayers where you're dealing with serialized transactions that haven't been landed, but that is a minority and those apps need to be more sophisticated anyway. The comparison to codecs makes sense too.
73044fd to
a735334
Compare
01380d2 to
f1998fe
Compare
| @@ -46,6 +49,11 @@ type VersionedCompiledTransactionMessage = BaseCompiledTransactionMessage & | |||
| version: number; | |||
| }>; | |||
|
|
|||
| const EMPTY_BLOCKHASH_LIFETIME_CONSTRAINT = { | |||
| blockhash: '11111111111111111111111111111111', | |||
| lastValidBlockHeight: 0n, | |||
There was a problem hiding this comment.
I know that getCompiledLifetimeToken() ignores this value so it can be literally anything, but this is maybe confusing to the reader (ie. that it's here).
We could obviate the need for this by doing…
const LIFETIME_TOKEN_WHEN_LIFETIME_ABSENT = '11111111111111111111111111111111';
/* ... */
lifetimeToken: transactionMessage.lifetimeConstraint
? getCompiledLifetimeToken(transactionMessage.lifetimeConstraint)
: LIFETIME_TOKEN_WHEN_LIFETIME_ABSENT,| return { | ||
| ...(transactionMessage.version !== 'legacy' | ||
| ? { addressTableLookups: getCompiledAddressTableLookups(orderedAccounts) } | ||
| : null), | ||
| header: getCompiledMessageHeader(orderedAccounts), | ||
| instructions: getCompiledInstructions(transactionMessage.instructions, orderedAccounts), | ||
| lifetimeToken: getCompiledLifetimeToken(transactionMessage.lifetimeConstraint), | ||
| lifetimeToken: getCompiledLifetimeToken(lifetimeConstraint), |
There was a problem hiding this comment.
Honestly, I thought we'd just make this optional on CompiledTransactionMessage and then make the compiler (ie. getCompiledTransactionMessageEncoder()) encapsulate the behaviour of filling it in when absent.
Actually I think I like that better, because then we're not passing a value with a "11111111111111111111111111111111" in it around your app.
There was a problem hiding this comment.
Yeah, actually maybe I take that back.
There's two levels of compiling here:
- From a nicely formatted message to the unreadable garbage with account indexes and stuff (ie.
compileTransactionMessage()) - From the garbage format to wire format (ie.
getCompiledTransactionMessageEncoder())
I think the behaviour of ‘fill it in with zeros’ should be done at the very latest opportunity – the one where we no longer have a choice. The higher level data structure should just not have a lifetimeToken property if there's no lifetime, rather than to fill it in with a placeholder.
a735334 to
5c49737
Compare
f1998fe to
42bc80f
Compare
|
@steveluscher Hmm implementing this is making me doubt this is the right decision. It makes the whole "what happens when we decompile" problem even deeper — unless we recognise "zeroes" as an empty sigil which you said you'd rather avoid. Consider the following encoding (A) and decoding (B) paths. When we don't have a a lifetime, we get the following back and forth:
I know we currently have this issue anyway, that is:
But I found there is something weird about optionalising the lifetime attribute of the What do you think? P.S.: I submitted a PR that tries this strategy but with a zero sigil to avoid the issue above. Lmk what you think. |
|
Taking a step back, do we actually need to be able to get to The use case mentioned was if you have a transaction modifying signer, it might add a lifetime. So you want to be able to compile without having one so the signer can take care of it. We also know there are cases where an app might use specific logic to 'fake' its lifetime, but because of the limitation of the bytecode format it does have a lifetime when compiled. Can we differentiate the signer case further here? If I'm just using But if I'm using signers then I can enter the compile flow without a lifetime. The transaction modifying signers get their chance to modify the transaction, and one of them might add a lifetime. Maybe it's an app-specific special case, maybe it's a real one. But if they add a lifetime then great, that ends up in the The design challenge here is the signer interface operates on |
|
Hey @mcintyre94, I'm not sure what you are suggesting about the Signer API, but I believe this was tackled on this PR. Unless you had something different in mind? Regarding the lack of |
|
I think my suggestion would be a larger change to I think the only case where this is really necessary is when you're using signers, and a signer is going to modify the transaction. Anything else, like excluding the lifetime for Solana Pay, is app-specific logic IMO. So I think you could make a signer method something like:
The concrete API change would be to signers, a modifying signer would need to have a function with the signature |
|
@mcintyre94 Oof this is a very different direction haha. I don't particularly agree with your premise that this is only something only the Signer API must handle. There are valid use cases that require apps to compile transaction messages before knowing which lifetime they are going to use. For instance, if they want to figure out the size of that transaction message to figure out if they can fit more instructions into it. Or anything that they could do with the Signer API — e.g. sending one to a wallet — but directly with the wallet's API. I do believe there is a strong need for Regarding the suggested |
42bc80f to
639657c
Compare
5c49737 to
2d89a8a
Compare
|
@steveluscher I'm not sure if you noticed but the next PR of this stack now tackles the changes we discussed. Lmk if that's also what you expected. 🙏 |
639657c to
fc3920b
Compare
2d89a8a to
5f77a74
Compare
Merge activity
|
5f77a74 to
ccad1a3
Compare
This PR removes the lifetime requirements on functions that compile transaction messages into transactions.
That is:
TransactionMessagewith no lifetime will return aTransactionwithout theTransactionWithLifetimeflag. However, the compiled bytes will contain 32 zeroes where the lifetime constraint is meant to be stored. (new behaviour)TransactionMessagewith an lifetime such asTransactionMessageWithBlockhashLifetimewill be forwarded to the returnedTransactiontype — e.g.TransactionWithBlockhashLifetime. (existing behaviour)TransactionMessagewith a loose lifetime — i.e.TransactionMessageWithLifetime— will be forwarded to the returnedTransactionType— i.e.TransactionWithLifetime. (existing behaviour).