feat: API to process multiple ixs with one TransactionContext

[Duckaet]

it implemenst the issue #185

[Duckaet]

applied the alternate approach #185 (comment)

added a basic test to verify instructions have shared account txn context

[buffalojoec]

Thanks for throwing this together! It's going to be a pretty hefty change - requiring lots of tests - so bear with me, but we can move toward a working solution.

[buffalojoec]

We need to actually capture all owners of each program in the top-level instruction set, so we'll want to use something like a Vec<(Pubkey, Pubkey)> where we pass (program_id, owner).

Then, we can stub only the top-level instructions' program accounts. What you have here won't work for a transaction context with two different program IDs invoked.

[buffalojoec]

This test is a good start, but we'll want to really expand on the test coverage for the new API method. It would be good to run a few instructions against a few different programs in the same transaction context. We also should have at least one test that runs up against the transaction context limits, like number of accounts or CU budget.

[buffalojoec]

I have a similar comment about duplicated code in this file. There's tons of duplication going on for the new API method you added. Let's see how we can share common code across all of the process_ functions.

[buffalojoec]

I'm still seeing lots of code being duplicated, even after the rebase.

[Duckaet]

sorry for the delay,

as the process_ and validate were nearly identical with some checks, i implemented helpers for single,chain and multi fn

passed the core fn to inner aswell to reuse the code

is this valid? i couldnt think of a better fix to code duplication

[Duckaet]

@buffalojoec , i tried to call compile_accounts for both the cases but it broke some tests.. because they build different KeyMaps for single vs all instruction the functionality couldn't be merged, do you have any other approach in mind

KeyMap::compile(instruction_index, std::iter::once(instruction))
KeyMap::compile_for_chain(instructions.iter())

extracted out functions from process_transaction_instructions and validate

fixed the program owner bug, added more tests to verify the functionality

i have reduced duplicate code aswell with extract func

[buffalojoec]

@Duckaet I just wrote a PR #187 that should make this change a lot easier. I refactored the way account fallbacks are handled during the account compilation step, which should enable you to share a KeyMap more easily.

[Duckaet]

@Duckaet I just wrote a PR #187 that should make this change a lot easier. I refactored the way account fallbacks are handled during the account compilation step, which should enable you to share a KeyMap more easily.

Thank you! that was logical to do so now i can map fallback_accounts instead of program_owners.
i will rebase asap with changes.. or should i copy the changes?

[buffalojoec]

@Duckaet I just wrote a PR #187 that should make this change a lot easier. I refactored the way account fallbacks are handled during the account compilation step, which should enable you to share a KeyMap more easily.

Thank you! that was logical to do so now i can map fallback_accounts instead of program_owners. i will rebase asap with changes.. or should i copy the changes?

Go ahead and rebase. I just merged.

[Duckaet]

@Duckaet I just wrote a PR #187 that should make this change a lot easier. I refactored the way account fallbacks are handled during the account compilation step, which should enable you to share a KeyMap more easily.

Thank you! that was logical to do so now i can map fallback_accounts instead of program_owners. i will rebase asap with changes.. or should i copy the changes?

Go ahead and rebase. I just merged.

I have implemented

1. the new fallback_accounts to unify the compile_accounts
2. reduced the duplicate code with some common functions in validate fn
3. tests are passing, should i introduce new tests now?

edit: added docs to new api to show differences in both process_

[buffalojoec]

Hey @Duckaet sorry to do this to you again, but I just merged #189, and that changed the account compilation API to just use a Message directly. The good news is that this simplified the account compilation quite a bit, and it's already using a transaction primitive! So now it should be even easier to implement your new API. However, you'll have to rebase again.

[Duckaet]

Hey @Duckaet sorry to do this to you again, but I just merged #189, and that changed the account compilation API to just use a Message directly. The good news is that this simplified the account compilation quite a bit, and it's already using a transaction primitive! So now it should be even easier to implement your new API. However, you'll have to rebase again.

just rebased it, I passed the SanitizedMessage to prepare_next_top_level_instruction() now, but their is still alot of duplication in lib.rs

should I also push the changes for the process_* and process_and_validate_* pairs using an inner helper with optional checks?

[buffalojoec]

@Duckaet thanks for the continued effort on this feature! It turned out to be much more of a beast than I anticipated, so I hopped in to assist.

I've added several new commits, the first of which reverts the harness back to what's on main. Then I refactored process_instruction_inner as I mentioned in my comment to be process_transaction_message and slowly, commit-by-commit, factored our a few setup and deconstruction steps into each caller.

This allowed me to implement both new API methods with shared code, and even clean up quite a bit of duplicated code!

Have a look when you get a chance.