fix(test/e2e): name cannot be empty error #3706
Conversation
rootulp
added
the
backport:v2.x
rootulp
requested review from
evan-forbes and
ninabarbakadze
and removed request for
a team
WalkthroughWalkthroughThe recent changes introduce a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Testnet
participant Genesis
participant Account
User->>Testnet: Call CreateAccount(name, pubkey, balance)
Testnet->>Genesis: Add Account (with Name)
Genesis->>Account: ValidateBasic()
Account-->>Genesis: Validation Result
Genesis-->>Testnet: Add Account Result
Testnet-->>User: CreateAccount Result
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 Configuration File (
|
| @@ -122,6 +122,9 @@ func (g *Genesis) WithKeyringAccounts(accs ...KeyringAccount) *Genesis { | |||
|
|
|||
| // AddAccount adds an existing account to the genesis. | |||
| func (g *Genesis) AddAccount(account Account) error { | |||
| if err := account.ValidateBasic(); err != nil { | |||
There was a problem hiding this comment.
are there any scenarios where this could consensus breaking? if so, are those scenarios relevant and do we need to document this somewhere?
There was a problem hiding this comment.
Why would this be consensus breaking? This is just about creating the genesis - I don't quite see how it affects actual running of a chain
evan-forbes
removed
the
backport:v2.x
|
tentatively removed the backport command out of an abundance of caution, but we can add it back if we don't need to be worried about this |
| @@ -122,6 +122,9 @@ func (g *Genesis) WithKeyringAccounts(accs ...KeyringAccount) *Genesis { | |||
|
|
|||
| // AddAccount adds an existing account to the genesis. | |||
| func (g *Genesis) AddAccount(account Account) error { | |||
| if err := account.ValidateBasic(); err != nil { | |||
There was a problem hiding this comment.
Why would this be consensus breaking? This is just about creating the genesis - I don't quite see how it affects actual running of a chain
| err = t.genesis.AddAccount(genesis.Account{ | ||
| PubKey: pk, | ||
| Balance: tokens, | ||
| Name: name, | ||
| }) |
There was a problem hiding this comment.
[note to self] this may be why it is preferable to use a struct constructor like NewGenesisAccount instead of creating a struct via genesis.Account{} in many files. That way the PR that added a new field to the struct can add it to the NewGenesisAccount constructor and doesn't have to CMD + F for all instances of the struct in the entire codebase to make sure one isn't missed.
Would be nice if Go compiler complained that the field wasn't initialized.
|
I think this can be backported to v2.x b/c AFAICT it only impacts |
rootulp
added
the
backport:v2.x
Recent e2e tests failed with: ``` 2024/07/18 17:22:08 Failed to setup testnet: converting accounts into sdk types: invalid account 4: name cannot be empty ``` because #3690 merged. Since e2e tests aren't run as part of CI on each PR, I didn't learn about the failure until @ninabarbakadze pinged about it. ## Testing ``` make test-e2e ``` gets past that error locally. (cherry picked from commit acf7fc1)
Recent e2e tests failed with:
because #3690 merged. Since e2e tests aren't run as part of CI on each PR, I didn't learn about the failure until @ninabarbakadze pinged about it.
Testing
gets past that error locally.