-
Notifications
You must be signed in to change notification settings - Fork 78
Deprecate CompilableTransactionMessage
#584
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
Conversation
🦋 Changeset detectedLatest commit: 1af5afc The changes in this PR will be included in the next version bump.
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. |
CompilableTransactionMessage
BundleMonFiles updated (4)
Unchanged files (123)
Total files change -582B -0.16% Final result: ✅ View report in BundleMon website ➡️ |
b01ee8e
to
7601485
Compare
Documentation Preview: https://kit-docs-ayvdsulmd-anza-tech.vercel.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just expand on the tests a bit to show ‘lifetime absent’ messages getting treated properly?
it('asks for a replacement blockhash even when the transaction message has a blockhash lifetime', () => { | ||
const transactionMessage = { | ||
...mockTransactionMessage, | ||
lifetimeConstraint: MOCK_BLOCKHASH_LIFETIME_CONSTRAINT, | ||
}; | ||
|
||
getComputeUnitEstimateForTransactionMessage_INTERNAL_ONLY_DO_NOT_EXPORT({ | ||
rpc, | ||
transactionMessage: { | ||
...mockTransactionMessage, | ||
lifetimeConstraint: MOCK_BLOCKHASH_LIFETIME_CONSTRAINT, | ||
}, | ||
transactionMessage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per #581, this is one of those opportunities where we can just leave the lifetimeConstraint
out rather than to supply a fake one right? ie. this should be split into two tests, one where you don't supply one at all, and one where you supply one that just gets written over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's gone sir. 🫡
// HACK: Since the `compileTransaction()` method will not compile a transaction with no lifetime we | ||
// supply a dummy lifetime. | ||
const INVALID_BUT_SUFFICIENT_FOR_COMPILATION_BLOCKHASH = { | ||
blockhash: '11111111111111111111111111111111' as Blockhash, | ||
lastValidBlockHeight: 0n, // This is not included in compiled transactions; it can be anything. | ||
} as const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9a4e686
to
75cd14a
Compare
6e65787
to
df62f6d
Compare
75cd14a
to
41452b8
Compare
df62f6d
to
2f5b244
Compare
41452b8
to
721dacd
Compare
2f5b244
to
49de3a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
721dacd
to
a0cbc0d
Compare
49de3a2
to
2e02696
Compare
Merge activity
|
2e02696
to
1af5afc
Compare
This PR adjusts some of the changesets of a [previously merged PR stack](#584) such that `major` bumps are used when breaking changes are introduced.
This PR deprecated the
CompilableTransactionMessage
type and removes internal usages of that type.