-
Notifications
You must be signed in to change notification settings - Fork 80
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
cluster: refactor base64 fields to base58 #851
Conversation
Codecov Report
@@ Coverage Diff @@
## main #851 +/- ##
==========================================
- Coverage 55.26% 55.22% -0.04%
==========================================
Files 112 113 +1
Lines 11628 12008 +380
==========================================
+ Hits 6426 6632 +206
- Misses 4268 4425 +157
- Partials 934 951 +17
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some small comments
cluster/definition.go
Outdated
v1_2 = "v1.2.0" // WIP | ||
v1_1 = "v1.1.0" | ||
v1_0 = "v1.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add enum for these versions, which would work as order of priority of versions at the time of comparison, which can be generated by go generate like we have in tracker as component enum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will also make it easy to add versions in future with enum approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually not sure this is going to work, since we need to do string comparison, which doesn't work nice with enums.
cluster/definition.go
Outdated
if err != nil { | ||
return nil, errors.Wrap(err, "marshal lock") | ||
} | ||
if isJSONv1x1(d.Version) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch case would be more cleaner + readable imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair
// distValidatorJSONv1x2 is the json formatter of DistValidator for versions v1.2.0 and later. | ||
type distValidatorJSONv1x2 struct { | ||
PubKey ethHex `json:"distributed_public_key"` | ||
PubShares []base58 `json:"public_shares,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we changing format of PubShares?
Since PubShares are internal public keys of individual co-validator we won't be using it anyways anywhere. Seems like a premature change.
we can keep it the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are upgrading all base64 types to either base58 or hex. It is in the ticket.
need to run |
Introduces draft cluster config version
1.2.0
. EIP712 signatures are refactored to industry standard hex. Other base64 fields are refactored to base58 to be URL safe and easy for humans to work with.v1.2.0
will contain more upgrades, so not "released" yet.category: feature
ticket: #848