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

Dropped basic wallet account from genesis file #510

Merged
merged 5 commits into from
Oct 3, 2024

Conversation

polydez
Copy link
Contributor

@polydez polydez commented Oct 2, 2024

Resolves: #446

In this PR we remove support of basic wallet creation in genesis config.

@polydez polydez marked this pull request as ready for review October 2, 2024 08:45
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

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

LGTM just the one question

Comment on lines 20 to 22
pub enum AccountInput {
BasicWallet(BasicWalletInputs),
BasicFungibleFaucet(BasicFungibleFaucetInputs),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove the possibility entirely like this? i.e. is there zero value in having a wallet account in the genesis file?

(I think yes, but just checking)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about it and decided that for mainnet we will need to predeploy different accounts and contracts (like DAO for initial funds management), but I don't think that predeployment of simple wallets has any sense for us except testing (but for testing purposes we can deploy wallets in tests).

@bobbinth bobbinth requested a review from igamigo October 2, 2024 15:13
@bobbinth
Copy link
Contributor

bobbinth commented Oct 2, 2024

Added @igamigo to reviewers as this will likely affect integration tests on the client.

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! I'll follow up with a small PR on the client (should be very small and quick to address so feel free to merge this anyway).

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: igamigo <ignacio.amigo@lambdaclass.com>
@polydez
Copy link
Contributor Author

polydez commented Oct 3, 2024

@bobbinth should we merge this PR or you would also like to review?

@polydez polydez merged commit a0150ee into next Oct 3, 2024
8 checks passed
@polydez polydez deleted the polydez-drop-genesis-wallet branch October 3, 2024 15:59
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.

4 participants