*: convert to uint64 #2668
*: convert to uint64 #2668
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2668 +/- ##
==========================================
- Coverage 53.00% 52.94% -0.06%
==========================================
Files 202 202
Lines 28006 28011 +5
==========================================
- Hits 14844 14830 -14
- Misses 11301 11321 +20
+ Partials 1861 1860 -1
☔ View full report in Codecov by Sentry. |
| @@ -743,7 +743,7 @@ func fmtStepPeers(step roundStep) string { | |||
|
|
|||
| // leader return the deterministic leader index. | |||
| func leader(duty core.Duty, round int64, nodes int) int64 { | |||
There was a problem hiding this comment.
why can't this function return uint64?
There was a problem hiding this comment.
it can, but uint64 is not really required by the code that uses this leader helper function
I have tried to use uint64s only where's necessary (most for spec compliance)
| @@ -319,11 +319,11 @@ func (s *Scheduler) resolveAttDuties(ctx context.Context, slot core.Slot, vals v | |||
| continue | |||
| } | |||
|
|
|||
| duty := core.NewAttesterDuty(int64(attDuty.Slot)) | |||
| duty := core.NewAttesterDuty(uint64(attDuty.Slot)) | |||
There was a problem hiding this comment.
Can attDuty.Slot be moved to uint64 as well?
There was a problem hiding this comment.
attDuty.Slot is of eth2p0.Slot type. Since, NewAttesterDuty(slot uint64), we need to typecast the eth2p0.Slot to uint64 type
| @@ -29,7 +30,7 @@ func StartDutyTrace(ctx context.Context, duty Duty, spanName string, opts ...tra | |||
| ctx, outerSpan = tracer.Start(tracer.RootedCtx(ctx, traceID), fmt.Sprintf("core/duty.%s", strings.Title(duty.Type.String()))) | |||
| ctx, innerSpan = tracer.Start(ctx, spanName, opts...) | |||
|
|
|||
| outerSpan.SetAttributes(attribute.Int64("slot", duty.Slot)) | |||
| outerSpan.SetAttributes(attribute.Int64("slot", safeInt64(duty.Slot))) | |||
There was a problem hiding this comment.
Same here, i wonder if we can have Slot as uint64 and eventually do the safeint64 checks when creating the struct.
There was a problem hiding this comment.
attribute has only 3 methods: attribute.Int(), attribute.Int64() & attribute.String()
q: why would we need to perform safeint checks if we define slots are uint64, won't the compiler already catch this?
| @@ -131,7 +131,7 @@ func newExchanger(tcpNode host.Host, peerIdx int, peers []peer.ID, vals int, sig | |||
| // signatures of the group according to public key of each DV. | |||
| func (e *exchanger) exchange(ctx context.Context, sigType sigType, set core.ParSignedDataSet) (map[core.PubKey][]core.ParSignedData, error) { | |||
| // Start the process by storing current peer's ParSignedDataSet | |||
| duty := core.NewSignatureDuty(int64(sigType)) | |||
| duty := core.NewSignatureDuty(uint64(sigType)) | |||
There was a problem hiding this comment.
Would change sigType to uint64 instead.
There was a problem hiding this comment.
I don't think that's required, what'd be the reason to change type for sigType which is currently an int to uint64?
slots, committee indices etc should be uint64 because the spec mandates that.
xenowits
force-pushed
the
xenowits/int64-uint64
branch
from
733732d
to
09a368c
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
xenowits
added
the
merge when ready
Convert
Duty.Slotfrom int64 to uint64.This is recommended according to ethereum consensus specs: https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#custom-types
category: feature
ticket: #2658