Add InstructionPlan type and helpers
#533
Conversation
🦋 Changeset detectedLatest commit: 0895d75 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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 |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BundleMonFiles added (3)
Unchanged files (127)
Total files change +2.55KB +0.74% Final result: ✅ View report in BundleMon website ➡️ |
lorisleiva
force-pushed
the
06-05-add_instructionplan_type_and_helpers
branch
3 times, most recently
from
e5531e9 to
4b471ec
Compare
| // TODO(loris): Either remove lifetime constraint or use the new | ||
| // `fillMissingTransactionMessageLifetimeUsingProvisoryBlockhash` | ||
| // function from https://github.com/anza-xyz/kit/pull/519. | ||
| m => | ||
| setTransactionMessageLifetimeUsingBlockhash( | ||
| {} as Parameters<typeof setTransactionMessageLifetimeUsingBlockhash>[0], | ||
| m, | ||
| ), |
There was a problem hiding this comment.
@steveluscher Here's an example showing how coupled the CompilableTransactionMessage type is to this instruction plan system.
|
Documentation Preview: https://kit-docs-ewgovoqca-anza-tech.vercel.app |
lorisleiva
force-pushed
the
06-05-add_instructionplan_type_and_helpers
branch
from
4b471ec to
0895d75
Compare
lorisleiva
force-pushed
the
06-04-add_empty_solana_instruction-plans_package
branch
from
4a0d43d to
d8a4286
Compare
There was a problem hiding this comment.
Let's talk about this iterator design. Check some assumptions for me.
- The iterator produces instructions. Given a transaction message, it will produce the next instruction or
nullif it doesn't fit in the supplied message. - The
getAll()method doesn't consider any one transaction message; it just presumes infinite space for instructions and gives them to you without complaining. - As far as I can tell, the only use case for obtaining an instruction is to append it to the same transaction message you passed to
next().
If those are even close to true, could the API instead take in a transaction message, and do one of two things:
- Pack all the instructions into a copy of the message then return that.
- Throw an error with:
- A new message with as many instructions packed into it as would fit.
- A new
InstructionPlanyou can use to pack the remaining ones.
// https://codesandbox.io/p/sandbox/generator-for-instruction-plan-24rpp8
function getWhateverWeCallThis<TInstruction extends Instruction = Instruction>(
instructions: TInstruction[]
) {
return {
packThatMessage(message: BaseTransactionMessage) {
let out = [message];
for (let ii = 0; ii < instructions.length; ii++) {
out.push(`instruction-${instructions[ii]}`);
if (ii === 2) {
throw new ItDidNotFitError(out.join(","), instructions.slice(ii + 1));
}
}
return out.join(",");
},
[Symbol.iterator](): Iterator<string> {
let ii = 0;
return {
next() {
if (ii == instructions.length) {
return {
value: undefined,
done: true,
};
}
return {
value: `instruction-${ii++}`,
done: false,
};
},
};
},
};
}
let thing = getWhateverWeCallThis([
"doThis",
"doThat",
"doTheOtherThing",
"doOneTooManyThings",
]);
console.log("--- Get all the instructions");
const allInstructions = Array.from(thing);
console.log({ allInstructions });
console.log("--- Pack the instructions into a message");
let message = "message";
while (true) {
try {
const packedMessage = thing.packThatMessage(message);
console.log("packedMessage", packedMessage);
break;
} catch (e) {
if (e instanceof ItDidNotFitError) {
console.log(
"At least we managed to pack this so far",
e.whateverWePackedSoFar
);
thing = e.continuationThing;
message = "newMessage";
continue;
}
throw e;
}
}Also, the ‘thing’ can implement the Iterator protocol, so that you can get all the instructions in the normal way (ie. passing the Iterator to Array.from()).
Am I too far off?
| expect(iterator.hasNext()).toBe(false); | ||
| expect(iterator.next(message)).toBeNull(); | ||
| }); | ||
| it('offers a `getAll` method that packs everything into a single instruction', () => { |
There was a problem hiding this comment.
Is this necessarily true, or is it just that the one in this test happens to pack it into a single instruction?
|
Let me just step back a bit and add more context on the Without the To illustrate this, let’s look at the Now, imagine we want to create a helper function that returns an Since the user will provide the program binary to deploy — in the
So instead of providing a static array of Let’s see how this works with a concrete example using the current architecture. First of all, imagine we are in the middle of planning a
That is, we are inside a Now say that the next For this example, we’ll assume there is a total of 1000 bytes we need to write to our buffer account. Looking at the current state of our transaction messages, we can see that the ideal scenario would be:
This is exactly how the base First, we call
As you can see, before trying a new message to put our
We are left with the following
Now, regarding the
As mentioned though, I think there is a way to remove that Now, to go back to your message. First of all, your assumptions were all correct. However, the example you used to reason with The proposed solution essentially makes the following changes:
There is likely a better design out there but I’m not sure that the |
|
Thanks for the detailed explainer! It helps a ton. I think these are my lingering concerns:
All of my suggestions above make the API more restrictive and less flexible, but with maybe fewer ways to screw it up and a little more efficiency. If we adopted those changes, can you think of any use cases that would prohibit? I think your example here would become: while (!messagePacker.done()) {
for (const candidate in candidates) {
try {
(candidate as Mutable<SingleTransactionPlan>).message =
messagePacker.packNextInstructions(candidate.message);
} catch (e) {
if (
isSolanaError(e, SOLANA_ERROR__MESSAGE_PACKER__NO_ROOM_LEFT_AT_THE_INN)
) {
const newPlan = await context.createSingleTransactionPlan(
[],
context.abortSignal
);
transactionPlans.push(newPlan);
candidates.push(newPlan);
continue;
}
throw e;
}
}
} |
|
Ooh I like that. Let me tackle your points one by one.
I think the provided example is perfect and I’ll work on updating the prototype first to see if there are any gotchas we’re not seeing before updating this stack. Going back to the naming convention (2), I think What about something like: type MessageAssemblerInstructionPlan = {
getMessageAssembler: () => MessageAssembler;
}
type MessageAssembler = {
done: () => boolean;
assembleNext(message: TransactionMessage) => TransactionMessage
} |
|
‘Assemble’ sort of gives the impression that something is being made from scratch, so I'm not sure.
‘Pack’ maybe has precedent 'round these parts, as in ‘pack a block,’ so maybe people understand what ‘packing’ is in this context. |
This PR adds the
InstructionPlantype and a variety of helpers to createInstructionPlansof different kinds.