-
Notifications
You must be signed in to change notification settings - Fork 42
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
Extend AccountProcedureIndexMap
with foreign procedures
#940
Conversation
Current state is extremely raw, but I managed to rework the test to make sure that current approach is working. Edit: I noticed that usage of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Not a full review, but I left some comments inline. The main thing is that I don't think we need to rely on the advice provider to build procedure maps for foreign accounts.
@bobbinth based on your comments I have questions how should we provide the information about foreign account (including foreign procedures). Right now it works like this:
But I see that this scheme should be reworked, and based on your comments the main think we should define is how do we want to provide the information about entire foreign account to the executor. Now it passed through the advice inputs, but since we shouldn't provide the foreign account procedure data using them, should we change the way we do that for the entire foreign account? Or otherwise, will we have to provide foreign account data except the procedures through the advice inputs, and procedures data through, for example, transaction arguments separately? For now I think we can provide the foreign account data through the transaction arguments instead of the advice inputs — that would allow us to roll back the changes in the
But this approach will allow us to keep track only of account procedure data. But can we use it to store the entire account? That will solve the issue about how to provide the account data not using the advice provider. In that case we will be able to add the account code to the mast store and simultaneously store the maps representing the whole account data in the data store |
I've created #938 to solve this long-term, but for now we can go with a simpler solution.
We should still use the current approach for passing account data to the transaction executor (i.e., via the advice inputs). But this approach does not include passing full account code - and for this I created One of the things we can do in this method is to track all code commitments for loaded account code and then pass it to the host the way you do now. So a big part of what you have now should still work - but we do need to get rid of |
…mitments in TransactionExecutor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Looks good! I left some comments inline - but the overall structure seems correct.
I'm not sure what to do with tests: on one hand our previous version was able to check the memory status after the foreign procedure invocation, but on the other it was exceptionally useless in terms of checking the fact whether the correct foreign procedure was executed. Probably the best option will be to leave the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I made a small commit to clean up a few things - but otherwise, I think it is good to go.
Probably the best option will be to leave the
test_load_foreign_account_basic()
andtest_load_foreign_account_twice()
tests as they were before this PR just to check the correctness of the memory layout after FPI, and add a new test which essentially will be the same as the currenttest_load_foreign_account_twice()
test (perhaps only the transaction code could be simplified to execute only one foreign procedure).
Could we not add extra checks to the same test? I'd do some basic checks in this PR, but leave more robust testing for #917.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Wasn't too familiar with the context of this code but the changes make sense. I tested them and it seems the FPI test on the client now passes.
In this last commit I roll back the changes made to the test, since it makes sense to move them to the other PR, which will close the #917. |
This PR reworks the
AccountProcedureIndexMap
to store the index map of the foreign accounts.Closes: #934