Conversation
🦋 Changeset detectedLatest commit: 69aa8de 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 |
BundleMonFiles updated (3)
Unchanged files (127)
Total files change +2.64KB +0.76% 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
packages/instruction-plans/src/__tests__/instruction-plan-test.ts
Outdated
Show resolved
Hide resolved
|
Documentation Preview: https://kit-docs-34oc2muup-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?
packages/instruction-plans/src/__tests__/instruction-plan-test.ts
Outdated
Show resolved
Hide resolved
|
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. |
|
@steveluscher I have managed to remove the Before I update this PR stack, let me know that you think! Here are some helpful links:
|
|
Looks great.
|
lorisleiva
added
the
do-not-close
|
Thanks!
|
lorisleiva
force-pushed
the
06-04-add_empty_solana_instruction-plans_package
branch
from
d8a4286 to
d4c2f12
Compare
lorisleiva
force-pushed
the
06-05-add_instructionplan_type_and_helpers
branch
2 times, most recently
from
b111845 to
ebd9cbf
Compare
lorisleiva
force-pushed
the
06-05-add_instructionplan_type_and_helpers
branch
from
ebd9cbf to
624aeab
Compare
steveluscher
left a comment
steveluscher
left a comment
There was a problem hiding this comment.
Awesome! 🎉
You don't have to take any of my nits; all optional.
Nits like: I think in the same way that you don't fill water into a bucket (rather you pour it) you don't fill instructions into a message (you pack them).
packages/instruction-plans/src/__tests__/instruction-plan-test.ts
Outdated
Show resolved
Hide resolved
lorisleiva
force-pushed
the
06-05-add_instructionplan_type_and_helpers
branch
from
624aeab to
30c6d1b
Compare
lorisleiva
force-pushed
the
06-04-add_empty_solana_instruction-plans_package
branch
from
d4c2f12 to
e331172
Compare
|
Thank you for the thorough review, this PR is much better as result! |
lorisleiva
force-pushed
the
06-04-add_empty_solana_instruction-plans_package
branch
from
e331172 to
6b6a931
Compare
lorisleiva
force-pushed
the
06-05-add_instructionplan_type_and_helpers
branch
2 times, most recently
from
1db2aad to
e227adf
Compare
Merge activity
|
lorisleiva
changed the base branch from
06-04-add_empty_solana_instruction-plans_package
to
graphite-base/533
lorisleiva
force-pushed
the
06-05-add_instructionplan_type_and_helpers
branch
from
e227adf to
69aa8de
Compare
|
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. |
2 participants
This PR adds the
InstructionPlantype and a variety of helpers to createInstructionPlansof different kinds.