Skip to content
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

Enhance the FPI tests. #918

Closed
Fumuran opened this issue Oct 12, 2024 · 5 comments · Fixed by #1128
Closed

Enhance the FPI tests. #918

Fumuran opened this issue Oct 12, 2024 · 5 comments · Fixed by #1128
Assignees
Milestone

Comments

@Fumuran
Copy link
Contributor

Fumuran commented Oct 12, 2024

What should be done?

See #896 (comment)

How should it be done?

Create new tests which will test the required capabilities.

When is this task done?

The task is done when the tests will also check the correctness of the foreign procedure invocation from within another account as a part of a complete transaction.

Additional context

No response

@bobbinth bobbinth added this to the v0.7 milestone Nov 7, 2024
@bobbinth bobbinth modified the milestones: v0.7, v0.8 Jan 13, 2025
@Fumuran
Copy link
Contributor Author

Fumuran commented Feb 6, 2025

Currently we have these FPI tests:

  • "Memory" test, which checks the memory layout of the foreign account. It has three subtests which check:
    • Obtaining the storage item (checks the item value)
    • Obtaining the storage map item (checks the item value)
    • Obtaining the storage item twice (checks that we reused the account data which was loaded during the first item obtaining, asserting that the next account slot was uninitialized).
  • "Integration" test, which gets the storage item and storage map item, checking their correctness. As opposed to the "memory" test, it uses TransactionExecutor instead of TransactionContext to execute the transaction script.

I think the basic capabilities of obtaining values and map values are tested, the only thing I can imagine is to make sure that the script that checks whether this foreign account was already loaded is working correctly by creating more complex test and using several different accounts (for now we used only one). But it should be fine too, since we check the basic case in the "memory" test.

cc @bobbinth

@bobbinth
Copy link
Contributor

bobbinth commented Feb 6, 2025

I would add a test which used two different accounts. The test could work like so:

  • Make a call to account 1 - e.g., read storage value.
  • Make a call to account 2 - e.g., read storage value.
  • Make a call to account 1 again - e.g., read storage value.

This way we can check that handling of multiple accounts works correctly (and that we don't load account 1 twice for some reason).

@Fumuran Fumuran linked a pull request Feb 6, 2025 that will close this issue
@PhilippGackstatter
Copy link
Contributor

Testing for error conditions would also be useful. For example, to test that if a foreign account attempts to write to the native account it would fail. I'm not sure how easy or hard this is to setup, but it seems important to ensure.

@PhilippGackstatter
Copy link
Contributor

PhilippGackstatter commented Feb 7, 2025

What would also be nice to have is to setup a test with an account in which we insert a map item and then, in MASM, get that map item from the account via execute_foreign_procedure and assert it's correct.
As part of this, we do not load the entire storage map from the foreign account into the advice provider, but only the key-value pair of interest and its merkle path, which should be sufficient to retrieve it, afaict. This is something @Dominik1999 has asked about and it would be interesting to both test and to have as a (tested) example.

One way to do this might be to change get_mock_fpi_adv_inputs to take a list of map items we want to have included and then only selectively include those in the merkle store. Since the existing fpi test already has a get_map_item call, that might actually already cover the above.

@bobbinth
Copy link
Contributor

bobbinth commented Feb 9, 2025

Closed by #1128.

@PhilippGackstatter - should we create more targeted issues for the points your brought up above?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants