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 and add AccountBuilder::build_existing #969

Merged
merged 9 commits into from
Nov 20, 2024

Conversation

PhilippGackstatter
Copy link
Contributor

Changes

  • Remove AccountBuilder::build_testing and add AccountBuilder::build_existing. This means we have a clear separation now between AccountBuilder::build which builds a new account and is available generally and AccountBuilder::build_existing which builds an existing account and is available only under testing.
  • Make Account::initialize_from_components private and replace usages with AccountBuilder. It turns out that we don't need to build accounts with a specific AccountId anywhere in tests so those occurrences were changed to generate a new one instead.
  • The nonce field in the AccountBuilder was no longer needed so I removed it. The nonce is set in build and build_existing directly.
  • The methods on MockChain to add new accounts were all adding the build accounts to MockChain::available_accounts and some methods also to MockChain::pending_objects which seemed unnecessary. I'm not 100% sure whether this is fine though.

closes #949

Builds on top of #871 and should be merged after that one but is otherwise ready for review.

@igamigo igamigo force-pushed the igamigo-docs-tests branch 2 times, most recently from 348a905 to 8cf400d Compare November 13, 2024 18:50
Base automatically changed from igamigo-docs-tests to next November 14, 2024 20:19
@bobbinth bobbinth self-requested a review November 15, 2024 01:29
@PhilippGackstatter PhilippGackstatter force-pushed the pgackst-account-builder-fixes branch from 9d676a9 to 513694f Compare November 15, 2024 07:07
Copy link
Contributor

@bobbinth bobbinth left a 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 a could of small comments inline - but also, would be great for @igamigo to review the PR.

objects/src/accounts/builder/mod.rs Outdated Show resolved Hide resolved
miden-tx/src/testing/mock_chain/mod.rs Outdated Show resolved Hide resolved
};

self.available_accounts
.insert(account.id(), MockAccount::new(account.clone(), seed, authenticator));

self.add_account(account.clone());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we no longer need this?

Copy link
Contributor Author

@PhilippGackstatter PhilippGackstatter Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps @igamigo can clarify this. It's not clear to me when adding new accounts in the chain whether they must be added to both available_accounts and pending_objects or only the former.

It seems the former is sufficient, so since we're adding the account to available_accounts adding it to pending_objects seems unnecessary. And we're already not doing it for the "existing faucet" case (this is from current next), so I made it consistent:

self.available_accounts
.insert(account.id(), MockAccount::new(account.clone(), seed, authenticator));
MockFungibleFaucet(account)
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally the intent was to keep MockChain consistent with how the actual chain would evolve, (currently not entirely the case since some updates are not performed entirely as they should) considering things like updates to the account and note trees. For new accounts, I don't think it's needed to add the account to pending objects, but I believe I originally added it for convenience (since it did not hurt either).

@bobbinth bobbinth mentioned this pull request Nov 19, 2024
Copy link
Collaborator

@igamigo igamigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

};

self.available_accounts
.insert(account.id(), MockAccount::new(account.clone(), seed, authenticator));

self.add_account(account.clone());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally the intent was to keep MockChain consistent with how the actual chain would evolve, (currently not entirely the case since some updates are not performed entirely as they should) considering things like updates to the account and note trees. For new accounts, I don't think it's needed to add the account to pending objects, but I believe I originally added it for convenience (since it did not hurt either).

@PhilippGackstatter PhilippGackstatter merged commit b60cd4b into next Nov 20, 2024
9 checks passed
@PhilippGackstatter PhilippGackstatter deleted the pgackst-account-builder-fixes branch November 20, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants