-
Notifications
You must be signed in to change notification settings - Fork 59
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
Implement AccountComponent
s
#941
Conversation
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! Not a review, but I took a quick look and left some comments inline.
4cf0185
to
7422e80
Compare
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 left some comment inline. The two main things are:
- There seems to be some mixing of test and non-test code going on. We should separate the two so that we retain full functionality when the
testing
feature is not on. - I'm not sure we need the
AccountComponent
trait. But maybe there is a good reason for it.
904fbba
to
bdb5fa9
Compare
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 reviewed all non-test code (and some test code as well), and left some comments inline.
Overall, I think we are close to merging this. Some issues I bring up (e.g., refactoring Account
constructors and/or introducing non-test AccountBuilder
) is something we can address in a follow-up PR.
5b77f61
to
d73813d
Compare
@bobbinth I addressed all your comments now and am creating new issues for the leftover tasks. |
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 left just a few small comments inline.
Before merging this, @igamigo could you take a look what changes we need to make on the client side to make sure the client does not break?
// TODO: Temporary fix: Build a native account that has the same code commitment as the foreign | ||
// account which is required for this test to pass right now. | ||
let native_account_id = | ||
AccountId::try_from(ACCOUNT_ID_REGULAR_ACCOUNT_UPDATABLE_CODE_ON_CHAIN).unwrap(); |
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.
Is this still applicable? I thought it should be already fixed. cc @Fumuran
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.
Just rebased onto latest next and it still fails for me with the previous code because the foreign account's code isn't being loaded:
--- STDERR: miden-tx tests::kernel_tests::test_tx::test_load_foreign_account_twice ---
thread 'tests::kernel_tests::test_tx::test_load_foreign_account_twice' panicked at miden-tx/src/tests/kernel_tests/test_tx.rs:912:50:
called `Result::unwrap()` on an `Err` value: EventError("Code commitment 0xb9ee787ee40cfeef9b0b89046122bd0620a9bc309630277906c581da36ddd7c6 is not in the advice provider")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
If I understood @Fumuran correctly, we don't currently have a way to load foreign accounts in the TransactionContext
which is the problem here.
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.
we don't currently have a way to load foreign accounts in the TransactionContext which is the problem here
Exactly. The problem is that we can't provide the mast forest of the foreign account's procedures, so we can get the procedure indexes map value, and can't execute the foreign procedure itself. For that purpose we have a load_account_code()
procedure in the TransactionExecutor
, but we don't have anything similar for the TransactionContext
(by the way, should we implement such procedure for it?).
In the new version of the tests I will use the TransactionExecutor
instead of TransactionContext
, so this won't be a problem in future.
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.
It sounds easiest to just use the TransactionExecutor
then in the new tests if it already has the account loading functionality require for this.
// TODO: Temporary fix: Build a native account that has the same code commitment as the foreign | ||
// account which is required for this test to pass right now. | ||
let native_account_id = | ||
AccountId::try_from(ACCOUNT_ID_REGULAR_ACCOUNT_UPDATABLE_CODE_ON_CHAIN).unwrap(); |
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.
Same comment as above.
e9d29c4
to
ba15a7a
Compare
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.
All looks good! Thank you!
Description
This PR implements
AccountComponent
s. AnAccountComponent
defines aLibrary
of code and the initial value and types of theStorageSlot
s it accesses.One or more components can be used to built
AccountCode
andAccountStorage
.Each component is independent of other components and can only access its own storage slots. Each component defines its own storage layout starting at index 0 up to the length of the storage slots vector.
Components define the
AccountType
s they support, meaning whether the component can be used to instantiate an account of that type. For example, a component implementing a fungible faucet would only specify support forAccountType::FungibleFaucet
. Using it to instantiate a regular account would fail. By default, the set of supported types is empty, so each component is forced to explicitly define what it supports.The PR implements three pre-defined components:
BasicWallet
,RpoFalcon512
,BasicFungibleFaucet
.These can be combined to build an account from components. Typical usage would look like this:
Other Noteworthy Changes
validate_procedure_metadata
and consistently implement the rule that procedures that do not access storage have offset and size set to 0 (for faucets this was (1, 1) previously).AccountCode::{compile, new}
as it should typically be instantiated usingAccount::initialize_from_components
orAccountCode::from_components
as a fallback.AccountCode
was replaced with components, oftenAccountMockComponent
.AccountComponent::new
, but it seems convenient to not do this and only do it inAccountCode::from_components
for two reasons:impl From<MyComponent> for AccountComponent
sinceAccountComponent::new
is infallible.closes #935