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

cmd: add-validators command #2306

Merged
merged 4 commits into from
Jun 14, 2023
Merged

Conversation

xenowits
Copy link
Contributor

Adds logic to charon alpha add-validators-solo command using mutable cluster APIs.

category: feature
ticket: #1887

@xenowits xenowits self-assigned this Jun 13, 2023
@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Patch coverage: 51.59% and project coverage change: +0.12 🎉

Comparison is base (bab124a) 53.79% compared to head (ee15244) 53.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2306      +/-   ##
==========================================
+ Coverage   53.79%   53.92%   +0.12%     
==========================================
  Files         191      191              
  Lines       25792    25977     +185     
==========================================
+ Hits        13876    14007     +131     
- Misses      10221    10255      +34     
- Partials     1695     1715      +20     
Impacted Files Coverage Δ
cmd/addvalidators.go 52.11% <51.59%> (+3.09%) ⬆️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -87,3 +112,22 @@ func TestValidateConfigAddValidators(t *testing.T) {
})
}
}

// TODO(xenowits): Add more extensive tests, this is just a very simple unit test.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add more tests in the subsequent PR

@corverroos
Copy link
Contributor

From ChatGPT:

Here are some suggestions based on the provided code:

cmd/addvalidators.go

  1. Line 12: Import declarations should be grouped in the following order:
    1. Standard library packages.
    2. Related third-party packages.
    3. Own organization's or application's packages.
  2. Line 26, Line 30: Provide comments for the TestConfig struct and its fields. This could help understand the code better.
  3. Line 60: It's usually a good idea to pass context.Context as the first argument to all functions that could potentially take a long time, or need to be cancellable or time out.
  4. Line 72: In Go, it's idiomatic to return the zero value when returning an error. It would be better to return nil, err here for consistency and to avoid confusion.
  5. Line 105: Here and in many other places, it's preferable to include the value that caused the error in the error message. This will help you debug issues much quicker.
  6. Line 124: Instead of repeating the loop of appending the single address to the list for each validator, consider using the repeat function from the strings package.
  7. Line 143: Don't ignore the error returned by the tbls.GenerateSecretKey() function. Always handle errors immediately, unless you're intentionally ignoring them (and if so, you should probably leave a comment explaining why).
  8. Line 223: The function runAddValidatorsSolo is quite large and complex, and performs many different tasks. Consider breaking it up into smaller functions, each of which performs one clear task.
  9. Line 247, Line 272: It's recommended to encapsulate the error handling within the called function itself, instead of returning errors and handling them in the caller.
  10. Line 285: Try to minimize the use of global state or functions that alter global state. Here, you're loading keys from files directly within this function. Instead, consider passing these keys into the function, so the function becomes easier to test.
  11. Line 343: Avoid hard-coding version strings. It would be a good idea to define it as a constant at the start of the file.
  12. Line 383, Line 385: These if-else statements can be a bit confusing. Consider refactoring them to make the code clearer.

Remember, code maintainability and readability are very important in long-term software projects. It's worth taking the time to refactor the code to make it easier to read and maintain.

cmd/addvalidators.go Outdated Show resolved Hide resolved
cmd/addvalidators.go Show resolved Hide resolved
cmd/addvalidators.go Outdated Show resolved Hide resolved
cmd/addvalidators.go Outdated Show resolved Hide resolved
cmd/addvalidators.go Outdated Show resolved Hide resolved
@xenowits xenowits added the merge when ready Indicates bulldozer bot may merge when all checks pass label Jun 14, 2023
@obol-bulldozer obol-bulldozer bot merged commit c8e5816 into main Jun 14, 2023
12 checks passed
@obol-bulldozer obol-bulldozer bot deleted the xenowits/add-vals-mutations branch June 14, 2023 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants