feat(types): Move deposits to new ssz lib #1810
Conversation
WalkthroughThe recent changes streamline the handling of deposits within the consensus module. Key modifications include the removal of redundant error checking in the Changes
Poem
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1810 +/- ##
==========================================
- Coverage 25.59% 25.58% -0.01%
==========================================
Files 327 328 +1
Lines 14484 14488 +4
Branches 21 21
==========================================
Hits 3707 3707
- Misses 10647 10651 +4
Partials 130 130
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (3)
- mod/consensus-types/pkg/types/body.go (1 hunks)
- mod/consensus-types/pkg/types/deposit.go (2 hunks)
- mod/consensus-types/pkg/types/deposits.go (1 hunks)
Additional comments not posted (19)
mod/consensus-types/pkg/types/deposits.go (3)
16-19: LGTM!The
SizeSSZfunction correctly calculates the SSZ encoded size for theDepositstype.
21-35: LGTM!The
DefineSSZfunction correctly defines the SSZ encoding for theDepositstype.
37-40: LGTM!The
HashTreeRootfunction correctly calculates the hash tree root for theDepositstype.mod/consensus-types/pkg/types/deposit.go (16)
Line range hint
41-48:
LGTM!The
NewDepositfunction correctly initializes aDepositstruct with the provided parameters.
Line range hint
50-55:
LGTM!The
Newfunction correctly delegates the creation of aDepositinstance to theNewDepositfunction.
Line range hint
57-66:
LGTM!The
VerifySignaturefunction correctly verifies the deposit data and signature using the providedsignatureVerificationFn.
Line range hint
70-76:
LGTM!The
DefineSSZfunction correctly defines the SSZ encoding for theDeposittype.
Line range hint
78-82:
LGTM!The
MarshalSSZfunction correctly marshals theDepositobject to SSZ format usingssz.EncodeToBytes.
Line range hint
84-86:
LGTM!The
UnmarshalSSZfunction correctly unmarshals theDepositobject from SSZ format usingssz.DecodeFromBytes.
Line range hint
88-90:
LGTM!The
SizeSSZfunction correctly returns the constantDepositSize.
Line range hint
92-94:
LGTM!The
HashTreeRootfunction correctly computes the Merkleization of theDepositobject usingssz.HashSequential.
Line range hint
98-104:
LGTM!The
MarshalSSZTofunction correctly marshals theDepositobject into a pre-allocated byte slice.
Line range hint
106-126:
LGTM!The
HashTreeRootWithfunction correctly hashes theDepositobject with a hasher using thefastsszlibrary.
Line range hint
128-130:
LGTM!The
GetTreefunction correctly hashes theDepositobject usingfastssz.ProofTree.
Line range hint
134-136:
LGTM!The
GetAmountfunction correctly returns the deposit amount in gwei.
Line range hint
138-140:
LGTM!The
GetPubkeyfunction correctly returns the public key of the validator specified in the deposit.
Line range hint
142-144:
LGTM!The
GetIndexfunction correctly returns the index of the deposit in the deposit contract.
Line range hint
146-148:
LGTM!The
GetSignaturefunction correctly returns the signature of the deposit data.
Line range hint
150-152:
LGTM!The
GetWithdrawalCredentialsfunction correctly returns the staking credentials of the deposit.
| return nil, err | ||
| } | ||
|
|
||
| layer[3] = Deposits(b.GetDeposits()).HashTreeRoot() |
There was a problem hiding this comment.
Re-add error checking for deposits' hash tree root calculation.
The removal of error checking for the deposits' hash tree root calculation might lead to unhandled errors. Consider re-adding the error checking to ensure robustness.
- layer[3] = Deposits(b.GetDeposits()).HashTreeRoot()
+ layer[3], err = Deposits(b.GetDeposits()).HashTreeRoot()
+ if err != nil {
+ return nil, err
+ }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range comments (3)
mod/consensus-types/pkg/types/deposit.go (2)
Line range hint
35-77:
Remove the redundantNewmethod.The
Newmethod is redundant as it callsNewDeposit. Consider removing it to avoid unnecessary duplication.-// New creates a new Deposit instance. -func (d *Deposit) New( - pubkey crypto.BLSPubkey, - credentials WithdrawalCredentials, - amount math.Gwei, - signature crypto.BLSSignature, - index uint64, -) *Deposit { - return NewDeposit( - pubkey, credentials, amount, signature, index, - ) -}
Line range hint
79-118:
LGTM! Consider adding comments for clarity.The SSZ encoding functions are implemented correctly. Adding comments to
DefineSSZwould improve readability.// DefineSSZ defines the SSZ encoding for the Deposit object. func (d *Deposit) DefineSSZ(c *ssz.Codec) { // Define the encoding for the Pubkey field ssz.DefineStaticBytes(c, &d.Pubkey) // Define the encoding for the Credentials field ssz.DefineStaticBytes(c, &d.Credentials) // Define the encoding for the Amount field ssz.DefineUint64(c, &d.Amount) // Define the encoding for the Signature field ssz.DefineStaticBytes(c, &d.Signature) // Define the encoding for the Index field ssz.DefineUint64(c, &d.Index) }mod/consensus-types/pkg/types/body.go (1)
Line range hint
40-315:
Re-add error checking for deposits' hash tree root calculation.The removal of error checking for the deposits' hash tree root calculation might lead to unhandled errors. Consider re-adding the error checking to ensure robustness.
- layer[3] = Deposits(b.GetDeposits()).HashTreeRoot() + layer[3], err = Deposits(b.GetDeposits()).HashTreeRoot() + if err != nil { + return nil, err + }
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (3)
- mod/consensus-types/pkg/types/body.go (3 hunks)
- mod/consensus-types/pkg/types/deposit.go (2 hunks)
- mod/consensus-types/pkg/types/deposits.go (1 hunks)
Additional comments not posted (8)
mod/consensus-types/pkg/types/deposits.go (2)
1-21: LGTM!The license header and package declaration are standard and correct.
23-30: LGTM!The imports and type alias are appropriate and correct.
mod/consensus-types/pkg/types/deposit.go (4)
Line range hint
1-21:
LGTM!The license header and package declaration are standard and correct.
Line range hint
23-33:
LGTM!The imports and constants are appropriate and correct.
Line range hint
120-153:
LGTM!The FastSSZ functions are implemented correctly.
Line range hint
155-199:
LGTM!The getters and setters are implemented correctly.
mod/consensus-types/pkg/types/body.go (2)
Line range hint
1-21:
LGTM!The license header and package declaration are standard and correct.
Line range hint
23-38:
LGTM!The imports and constants are appropriate and correct.
| // SizeSSZ returns the SSZ encoded size in bytes for the Deposits. | ||
| func (ds Deposits) SizeSSZ(bool) uint32 { | ||
| return ssz.SizeSliceOfStaticObjects(([]*Deposit)(ds)) | ||
| } | ||
|
|
||
| // DefineSSZ defines the SSZ encoding for the Deposits object. | ||
| func (ds Deposits) DefineSSZ(c *ssz.Codec) { | ||
| c.DefineDecoder(func(*ssz.Decoder) { | ||
| ssz.DefineSliceOfStaticObjectsContent( | ||
| c, (*[]*Deposit)(&ds), constants.MaxDepositsPerBlock) | ||
| }) | ||
| c.DefineEncoder(func(*ssz.Encoder) { | ||
| ssz.DefineSliceOfStaticObjectsContent( | ||
| c, (*[]*Deposit)(&ds), constants.MaxDepositsPerBlock) | ||
| }) | ||
| c.DefineHasher(func(*ssz.Hasher) { | ||
| ssz.DefineSliceOfStaticObjectsOffset( | ||
| c, (*[]*Deposit)(&ds), constants.MaxDepositsPerBlock) | ||
| }) | ||
| } | ||
|
|
||
| // HashTreeRoot returns the hash tree root of the Deposits. | ||
| func (ds Deposits) HashTreeRoot() common.Root { | ||
| return ssz.HashSequential(ds) | ||
| } |
There was a problem hiding this comment.
LGTM! Consider adding comments for clarity.
The SSZ encoding functions are implemented correctly. Adding comments to DefineSSZ would improve readability.
// DefineSSZ defines the SSZ encoding for the Deposits object.
func (ds Deposits) DefineSSZ(c *ssz.Codec) {
c.DefineDecoder(func(*ssz.Decoder) {
// Define the decoding logic for Deposits
ssz.DefineSliceOfStaticObjectsContent(
c, (*[]*Deposit)(&ds), constants.MaxDepositsPerBlock)
})
c.DefineEncoder(func(*ssz.Encoder) {
// Define the encoding logic for Deposits
ssz.DefineSliceOfStaticObjectsContent(
c, (*[]*Deposit)(&ds), constants.MaxDepositsPerBlock)
})
c.DefineHasher(func(*ssz.Hasher) {
// Define the hashing logic for Deposits
ssz.DefineSliceOfStaticObjectsOffset(
c, (*[]*Deposit)(&ds), constants.MaxDepositsPerBlock)
})
}
Summary by CodeRabbit
New Features
Depositstype for managing deposit entries with enhanced serialization and hashing capabilities.Bug Fixes
Refactor