fix: genesis account creation in e2e tests #3243
Conversation
There was a problem hiding this comment.
Thanks, Callum. After reviewing the new logic, I believe it addresses the issue with the SimpleE2ETest and duplicate accounts. However, I haven't tested it yet.Once it's merged, I'll run the e2e test and provide you with an update.
I ran the e2e test, and it worked with no error 👍 all good!
| @@ -20,40 +19,28 @@ const ( | |||
| DefaultInitialBalance = 1e15 // 1 billion TIA | |||
| ) | |||
|
|
|||
| // Account represents a user account on the Celestia network. | |||
| // KeyringAccount represents a user account on the Celestia network. | |||
There was a problem hiding this comment.
Perhaps this part of the comment does not apply anymore:
or an address needs to be provided
There was a problem hiding this comment.
[suggestion] The name account in KeyringAccount is a bit confusing, as it is more of some info about the account not the actual account. I'd suggest using an alternative name like KeyringAccountInfo.
There was a problem hiding this comment.
So I tried to distinguish KeyringAccount as an Account that is paired with a Keyring belonging to the Genesis struct. If you don't want to use the keyring provided by the Genesis struct then you use an Account and therefore must include the public key
There was a problem hiding this comment.
Thanks, your explanation has made things clearer. I think it would be helpful to also add these explanations as a comment to both structs.
cmwaters
requested review from
evan-forbes,
rootulp and
ninabarbakadze
as code owners
WalkthroughThe changes revolve around a significant refactor in how accounts and validators are managed within the test environment. The updates include a transition from direct public key access to utilizing a Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // KeyringAccount represents a user account on the Celestia network. | ||
| // Either the name, if using the genesis keyring, or an address | ||
| // needs to be provided | ||
| type Account struct { | ||
| type KeyringAccount struct { |
There was a problem hiding this comment.
Consider revising the comment for KeyringAccount to reflect the removal of the address requirement.
// KeyringAccount represents a user account on the Celestia network.
// Either the name, if using the genesis keyringCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // KeyringAccount represents a user account on the Celestia network. | |
| // Either the name, if using the genesis keyring, or an address | |
| // needs to be provided | |
| type Account struct { | |
| type KeyringAccount struct { | |
| // KeyringAccount represents a user account on the Celestia network. | |
| // Either the name, if using the genesis keyring | |
| type KeyringAccount struct { |
Closes: #3240
This is an attempt at fixing the problem @staheri14 pointed out with the creation of the validator accounts in the genesis file.
It differentiates between a
KeyringAccountwhich corresponds to the keyring of the genesis struct and theAccountwhich is required inDocument()to actually generate the genesis. This way users don't have to use the genesis' keyring if they don't want toNote: This also includes a small fix in
Document()whereby theInitialTokenswas actually not being used and the default was being used instead