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

Revisit Account::initialize_from_components #949

Closed
PhilippGackstatter opened this issue Nov 1, 2024 · 2 comments
Closed

Revisit Account::initialize_from_components #949

PhilippGackstatter opened this issue Nov 1, 2024 · 2 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@PhilippGackstatter
Copy link
Contributor

What should be done?

Revisit Account::initialize_from_components and whether it can be replaced with something else that is safer and has a better developer experience.
This function was introduced to have a central place to validate that:

  1. All components are valid for the to-be-built account type.
  2. that code and storage are initialized together and there is no room for user error if there were to separate methods on AccountCode and AccountStorage to initialize them separately.
  3. Code and storage need to be initialized before building an AccountId, so a method like Account::from_components(seed, &components) is technically feasible but not practical as the seed depends on the code and storage built from the components. So in practice, users would always have to build code and storage from the components first, then build the ID and then construct the account. Such a method would be redoing work and building AccountCode from components is somewhat expensive due to the call to MastForest::merge.

Problems with this method

  1. Still this method is somewhat unintuitive as it builds code and storage but is defined on Account itself.
  2. It only builds parts of the full account and it may not be immediately obvious how to use this method to build a full account.
  3. Finally, users are not prevented from passing this code and storage to Account::new using a seed that results in a different type than what was passed to Account::initialize_from_components. For instance, it would be possible to initialize code and storage for a faucet, but then use a seed that turns into a regular account. This would result in a broken account.

How should it be done?

We should investigate if there is a method for building accounts that could eliminate the above-mentioned weaknesses.
One potential route is to do all of the necessary tasks in one constructor, e.g. something like:

pub fn new(
    init_seed: [u8; 32],
    storage_mode: AccountStorageMode,
    account_type: AccountType,
    components: &[AccountComponent],
) -> Result<Self, AccountError> {
    let (code, storage) = Self::initialize_from_components(account_type, components)?;
    let seed = AccountId::get_account_seed(
        init_seed,
        account_type,
        storage_mode,
        code.commitment(),
        storage.commitment(),
    )?;
    let id = AccountId::new(seed, code.commitment(), storage.commitment())?;
    Ok(Account::from_parts(id, AssetVault::default(), storage, code, Felt::ZERO))
}

Perhaps we would then want another variant of this taking seed: Word, storage_mode: AccountStorageMode, account_type: AccountType, components: &[AccountComponent] which would validate that the resulting ID has the expected type and storage mode.

Another route is to remove constructors on Account directly and force users to build accounts using an AccountBuilder: #948

Any mix of these options is possible and there might be other options as well.

When is this task done?

This task is done when users can conveniently and safely (see problem 3 above) build Accounts.

Additional context

No response

@PhilippGackstatter PhilippGackstatter added the enhancement New feature or request label Nov 1, 2024
@bobbinth bobbinth added this to the v0.7 milestone Nov 7, 2024
@PhilippGackstatter
Copy link
Contributor Author

Follow-up from the AccountBuilder PR and this comment.
I'm adding it to this issue, since this one is fairly small, I would just settle on using the AccountBuilder where we currently use the initialize function and make it otherwise pub(super) so it is no longer part of the public API.

Both this issue and the follow-up task to refactor build_testing as build_existing are dependent on #871, as they would produces conflict with it and are made easier with that PR merged.

@PhilippGackstatter
Copy link
Contributor Author

Closed by #969.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

No branches or pull requests

2 participants