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.

Sign up for GitHub

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

Jump to bottom

Group Slashing Protection History Packages Idiomatically #9873

Merged
merged 26 commits into from
Nov 9, 2021
Merged

Group Slashing Protection History Packages Idiomatically #9873

merged 26 commits into from
Nov 9, 2021

Conversation

rauljordan
Copy link
Contributor

This PR changes our messy packaging of slashing protection JSON support from:

validator/
  slashing-protection/
    local/
      iface/
        protector.go
      format/
        format.go
    export_cli.go
    export.go
    import_cli.go
    import.go

to

cmd/
  validator/
    slashing-protection/
      export.go
      import.go
validator/
  slashing-protection-history/
    format/
      format.go
    export.go
    import.go

No logic changes are added in this PR

@rauljordan rauljordan requested a review from a team as a code owner November 9, 2021 01:41
@rauljordan rauljordan added OK to merge Ready For Review A pull request ready for code review labels Nov 9, 2021
@rauljordan rauljordan self-assigned this Nov 9, 2021
kasey
kasey previously approved these changes Nov 9, 2021
View reviewed changes
Copy link
Contributor

@kasey kasey left a comment

Choose a reason for hiding this comment

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

Very nice improvement. For extra credit I think the package names could be a bit more concise.

cmd/validator/slashing-protection/export.go Outdated
@@ -1,4 +1,4 @@
package slashingprotection
package slashingprotectioncmd
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a really long package name. Considering packages like this are only imported by the command that uses them, maybe we could call them something simple and generic like subcmd.

validator/slashing-protection-history/doc.go Outdated
// Package slashingprotection defines methods to parse, import, and export slashing protection data
// from a standard JSON file according to EIP-3076 https://eips.ethereum.org/EIPS/eip-3076. This format
// is critical to allow safe interoperability between Ethereum consensus clients.
package slashingprotectionhistory
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for making package comment files, seeing this is a good reminder for me to do more of that. I used to have a practice of always running godoc to browse how my final docs looked and think about whether I could learn to use a package from that viewpoint, but I've gotten way too lazy about that!

validator/slashing-protection-history/export_test.go Outdated
@@ -1,4 +1,4 @@
package interchangeformat
package slashingprotectionhistory
Copy link
Contributor

Choose a reason for hiding this comment

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

what about a shorter package name like history?

rkapka
rkapka requested changes Nov 9, 2021
View reviewed changes
cmd/validator/slashing-protection/export.go Outdated
package slashingprotection
package slashingprotectioncmd
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change? Other packages in cmd don't have the cmd suffix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is that it avoids conflicts with the slashingprotection package when importing it. For example:

import (
  // CONFUSION! Go imports autocomplete doesn't know what to do, need manual aliasing
  slashingprotection "github.com/prysm/cmd/validator/slashing-protection"
  slashingprotection "github.com/prysm/validator/slashing-protection"
)

vs

import (
  // Automatically aliased by IDE
  slashingprotectioncmd "github.com/prysm/cmd/validator/slashing-protection"
  slashingprotection "github.com/prysm/validator/slashing-protection"
)

cmd/validator/slashing-protection/export.go
"github.com/urfave/cli/v2"
)

const (
jsonExportFileName = "slashing_protection.json"
)

// ExportSlashingProtectionJSONCli extracts a validator's slashing protection
// Extracts a validator's slashing protection
Copy link
Contributor

Choose a reason for hiding this comment

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

The doc should begin with the function's name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not if the function is unexported

cmd/validator/slashing-protection/import.go
"github.com/urfave/cli/v2"
)

// ImportSlashingProtectionCLI reads an input slashing protection EIP-3076
// Reads an input slashing protection EIP-3076
Copy link
Contributor

Choose a reason for hiding this comment

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

The doc should begin with the function's name.

validator/slashing-protection-history/import_test.go Outdated
// We encode the standard slashing protection struct into a JSON format.
// We encode the standard slashing protection struct into a JSON
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

global find replace, will fix

validator/slashing-protection-history/import_test.go Outdated Show resolved Hide resolved
validator/slashing-protection-history/round_trip_test.go Outdated Show resolved Hide resolved
@prylabs-bulldozer prylabs-bulldozer bot merged commit 0fb9143 into develop Nov 9, 2021
@delete-merged-branch delete-merged-branch bot deleted the rename-slashing-protect branch November 9, 2021 16:49
This was referenced Feb 8, 2022
Merged
Merged
unixpi pushed a commit to status-im/nimbus-eth2 that referenced this pull request Feb 9, 2022
@jeremyschlatter
update prysm export docs (#3365)
47b1870 
This command was [recently renamed](prysmaticlabs/prysm#9873).
prestonvanloon pushed a commit to prysmaticlabs/documentation that referenced this pull request Feb 16, 2022
@jeremyschlatter
fix incorrect subcommand name in examples (#405)
47fc0b7 
* Revert "Update slashing-protection.md"

This reverts commit d24630d.

* slashing-protection -> slashing-protection-history

This name was changed in prysmaticlabs/prysm#9873.

csantana1121 updated the docs in #387, but unfortunately they only
updated the versioned docs without also updating the master docs, and
also incorrectly updated version 2.0.0, which did not have this name
change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkapka rkapka rkapka approved these changes

@kasey kasey kasey left review comments

@prestonvanloon prestonvanloon Awaiting requested review from prestonvanloon prestonvanloon is a code owner automatically assigned from prysmaticlabs/core-team

@terencechain terencechain Awaiting requested review from terencechain terencechain is a code owner automatically assigned from prysmaticlabs/core-team

Assignees

@rauljordan rauljordan

Labels
Ready For Review A pull request ready for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rauljordan @kasey @rkapka