cluster: fix hashing of legacy checksummed addresses #1156
Conversation
| PubShares: [][]byte{ | ||
| testutil.RandomBytes32(), // TODO(corver): Change sigs to Bytes48. | ||
| testutil.RandomBytes32(), | ||
| t.Run(version, func(t *testing.T) { |
There was a problem hiding this comment.
just wrapped this in a subtest
|
|
||
| // TestExamples tests whether charon is backwards compatible with all examples. | ||
| func TestExamples(t *testing.T) { | ||
| lockFiles, err := filepath.Glob("examples/*lock*") |
There was a problem hiding this comment.
added /examples folder which contains real world examples that we need to support
cluster/definition.go
Outdated
| @@ -103,10 +92,10 @@ type Definition struct { | |||
| Threshold int `json:"threshold" ssz:"uint64" config_hash:"5" definition_hash:"5"` | |||
|
|
|||
| // FeeRecipientAddress 20 byte Ethereum address. | |||
| FeeRecipientAddress []byte `json:"fee_recipient_address,0xhex" ssz:"Bytes20" config_hash:"6" definition_hash:"6"` | |||
| FeeRecipientAddress EthAddr `json:"fee_recipient_address,0xhex" ssz:"Bytes20" config_hash:"6" definition_hash:"6"` | |||
There was a problem hiding this comment.
we cannot use bytes for internal representation of addresses since that looses checksummed information, so store as string rather.
| // Field (0) 'Address' | ||
| hh.PutBytes([]byte(to0xHex(o.Address))) | ||
| continue | ||
| } |
There was a problem hiding this comment.
refactored if/else
Codecov ReportBase: 52.90% // Head: 52.95% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1156 +/- ##
==========================================
+ Coverage 52.90% 52.95% +0.04%
==========================================
Files 131 131
Lines 15301 15302 +1
==========================================
+ Hits 8095 8103 +8
+ Misses 6029 6019 -10
- Partials 1177 1180 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| "address": "", | ||
| "enr": "enr:-JG4QG2QA-0OTnzUAmWtYuuAJ5QwhQRQTT0pTBuapBHgU75sBAnBZieYVR8fpHMzRc66sqgDYkhMJicLdz1ejhXjRV-GAYJFBnlQgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQORVX5d5U7HULLj_qfh0qP6RrO35tieBwJcKGj4GNBaiYN0Y3CCDhqDdWRwgg4u", | ||
| "nonce": 0, | ||
| "config_signature": null, |
There was a problem hiding this comment.
is it possible to populate these "null" fields?
There was a problem hiding this comment.
These are examples of real world locks. I didn't generate them.
There was a problem hiding this comment.
Where did you get them from?
There was a problem hiding this comment.
our cluster, charon create cluster, and a user's cluster
corverroos
added
the
merge when ready
corverroos
force-pushed
the
corver/checkaddrs
branch
from
743a008
to
4dfb769
Compare
Fixes issue of "invalid lock hash" when older versions of cluster locks contains checksummed addresses.
category: bug
ticket: #1153