Update FPI tests

[Fumuran]

This PR updates the existing FPI tests and adds one additional.

• Existing tests were reworked to primarily check the correctness of the memory layout after the transaction execution.
• A new test perform a more comprehensive check, asserting the correctness of the execution of the foreign procedure.

Closes: #917

[PhilippGackstatter]

Looks good to me!

Seems like a good approach to test the memory layout using the TransactionContext and test the account loading and the procedure index map using TransactionExecutor.

Left some optional comments.

[PhilippGackstatter]

Just for my understanding: If this was initialized it would mean we would've loaded the foreign account a second time and thus we would have three accounts loaded in total (1 native account, 2 foreign accounts)?
Perhaps you can add a short comment to this assertion to explain this context.

[PhilippGackstatter]

Suggested change

	fn test_fpi_basic() {
	fn test_fpi_load_foreign_account_code() {

Nit: Suggestion for a more specific name for the test, but totally optional.

[Fumuran]

I totally agree that the name of this test should be updated to be more specific, but I'm not sure about the name. In fact this is an integration test, which checks the correctness of the foreign procedure execution on every stage: we load the foreign account code, provide the mast forest to the executor, construct the account procedure maps and execute the foreign procedure to obtain the data from the foreign account's storage slot. So I think that test_fpi_load_foreign_account_code is not the best option, because it reflects only one of the execution stages. May be something like test_fpi_execute_foreign_procedure? So the logic is that the execution of the foreign procedure is the result achieved by the all previous stages, so it's kind of illustrating the accomplished work

[PhilippGackstatter]

Sure, your suggestion sounds better to reflect the overall test better although it is somewhat generic, but still fits here. Together with a doc comment it's totally fine imo.

[PhilippGackstatter]

Nit: Same here - would be great if you could add a short doc comment explaining the two phases of the test.

[Fumuran]

During the rebase I noticed another issue, related to the FPI: in case a user wants to access the map storage slot, the internals of this map should be preliminarily loaded to the advice provider.
An easy way to solve it is to populate the advice provider the same way we do this for the account storage and code (as I implemented it for now), so we can add the maps only if user want to access the map slots, but it looks like not the best UX.
We could populate the advice provider with this data during the TransactionContext building, but potentially it could be expensive in memory terms. Not sure what is the best option.

[Fumuran]

Also I noticed two more "structural" problems:

1. Currently the TransactionContext have both advice_inputs and tx_args fields, although tx_args already contains the advice inputs internally. I think we should remove the advice_inputs field and leave only tx_args: it will simplify the advice inputs management, I will be able to remove the clumsy code which populates the tx arguments with the advice inputs (here).
2. I think that it is very useful to have an opportunity to load the foreign account code to the TransactionContext, but in case we use the TransactionExecutor we have to different ways to do the same thing — probably we can optimize it. One way is to always use the account codes stored in the TransactionContext and then populate them to the TransactionExecutor (since we use context in the executor anyway). So our new load_account_code() function will become an internal function, which will be used during the creation of the executor, and the main (and probably only) way to provide the account code will be using the context. I think it will make the code more simple and less confusing, also improving the UX.

[bobbinth]

Looks good! Thank you! Not a very deep review from me - but I left one question inline.

Also, we should probably merge this after #969 as I think we'll need to do some minor rebasing after this.

[igamigo]

Should this be deleted? Or could it be uncommented and then you could use TransactionContext::execute() unless I missed something

[Fumuran]

I left this line because I wanted to come to some decision about what I described in the second point of my comment. Because I think we can optimize the way we provide the foreign account code, so we will be able to use only foreign_account_codes function.

[bobbinth]

@Fumuran - now that #969 has been merged, could you resolve merge conflicts?

[Fumuran]

@phklive need this PR to be merged for his Oracle task, so I'll merge the current state into next, and address the issues I noted above in the follow up PR.