feat: Implement eth2util/deposit by iamquang95 · Pull Request #134 · NethermindEth/pluto

iamquang95 · 2026-01-22T10:43:17Z

Should fix: #63

varex83

Putting the first part of the review here, will check other part later

varex83 · 2026-01-22T11:08:00Z

+}
+
+/// Serializes a list of deposit data into a single file.
+pub fn marshal_deposit_data(


I think it should be replaced using serde and serde_json

The DepositDataJson itself use serde as the serialize/deserialize. This method contains the transformation from input to serde-able datastructure. I don't like the name marshal, but decided to keep it to compatible with golang API for the first phase

I agree with @varex83. Ideally this would be done at the serde level.

Not sure if I understand correctly, but I'm not sure how can I implement this at serde level.

The function needs to take network as an argument to get fork_version and sign on that. If we want to write a Serialization we need to wrap the DepositData to DepositDataWithNetwork and impl Serialize trait for that. But it's quite over-engineering from my point of view.

I don't think it will be over-engineering. From my point of view it would be just more rust idiomatic and will work natively with serde (which I would expect as a developer working on serialization/deserialization in rust)

Now that you mention the extra parameter, your solution seems like the right approach but I agree on the overengineering comment.

Let's leave it as it is for now then.

emlautarom1 · 2026-01-27T17:15:57Z

+}
+
+/// Serializes a list of deposit data into a single file.
+pub fn marshal_deposit_data(


I agree with @varex83. Ideally this would be done at the serde level.

emlautarom1

LGTM

Before merging, I suggest to evaluate if we can replace some of our code (in particular helpers) with methods provided by alloy

varex83 · 2026-01-29T15:24:56Z

+    tokio::fs::write(&file_path, bytes).await?;
+
+    let mut perms = tokio::fs::metadata(&file_path).await?.permissions();
+    perms.set_readonly(true);
+    tokio::fs::set_permissions(&file_path, perms).await?;


File could be modified between write and permission change. I think we can check file hash before / after or use some kind of atomic operations instead.

But it should be fine right? We read a snapshot of the permissions, and then write a modified version of the snapshot. If something happens to the file (ex. gets deleted) then the operation fails.

I mean that we set file to read-only, but the contents of it could be changed between write and permissions update

Hmm, makes sense. Let's follow what the Go implementation does and add a TODO here in case they don't do it atomically.

varex83 · 2026-01-29T15:27:21Z

+    let mut deposit_datas_list = Vec::new();
+
+    for file in files {
+        let bytes = tokio::fs::read(&file).await?;


Currently files are read sequentially, consider using FuturesUnordered

Potential optimization, but the original Go code performs this operation sequentially.

varex83 · 2026-01-29T15:31:54Z

+}
+
+/// Reads all deposit data files from a cluster directory.
+pub async fn read_deposit_data_files(cluster_dir: &Path) -> Result<Vec<Vec<DepositData>>> {


Suggested change

pub async fn read_deposit_data_files(cluster_dir: &Path) -> Result<Vec<Vec<DepositData>>> {

pub async fn read_deposit_data_files(cluster_dir: impl AsRef<Path>) -> Result<Vec<Vec<DepositData>>> {

Is there any lint that we can use for this? Seems to be a recurring topic in our reviews

Seems like adding pedantic = "warn" to [workspace.lints.clippy] adds a lot of good suggestions, I think we can use it

or we can customize github copilot review: https://docs.github.com/en/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review#customizing-copilots-reviews-with-custom-instructions

It would be nice to have immediate feedback on the IDE rather than waiting for Copilot to review it, but as a fall-back that's OK too.

…eposit2

varex83

LGTM!

varex83 · 2026-01-30T12:07:25Z

+    async fn read_deposit_data_files_invalid_pubkey_hex() {
+        let dir = tempdir().unwrap();
+        let datas = generate_deposit_datas(DEFAULT_DEPOSIT_AMOUNT);
+        let bytes = marshal_deposit_data(&datas, "goerli").unwrap();
+        let file = get_deposit_file_path(dir.path(), DEFAULT_DEPOSIT_AMOUNT);
+        tokio::fs::write(&file, &bytes).await.unwrap();
+


Since we have a lot of code duplication here I would suggest using test_case or something similar

That looks cleaner, will try to adopt it later 👍

iamquang95 added 6 commits January 21, 2026 18:41

feat: implement eth2util/deposit

d3af1aa

feat: use TreeHash

90d3eb4

feat: add eth2util/helpers

190355b

fix: centralize logic and errors

393b939

test: add test marshal_deposit_data

72a7e09

fix: move dependencies to workspace cargo.toml

0340e75

iamquang95 requested review from emlautarom1 and varex83 January 22, 2026 10:43

varex83 requested changes Jan 23, 2026

View reviewed changes

iamquang95 added 2 commits January 27, 2026 16:58

fix: address comments

8ceaee9

fix: add more test to deposit

c8bf970

emlautarom1 requested changes Jan 27, 2026

View reviewed changes

iamquang95 added 7 commits January 28, 2026 14:21

fix: move DepositData function to impl DepositData

09c043e

fix: define Result type at crate level

41456f6

feat: implement epoch_from_slot

795f1d0

fix: remove error helper functions

5c2f749

fix: read_deposit_data_files use async file

2096dcd

test: improve tests on deposit crate

fe326f5

test: fix test name

0346c07

emlautarom1 approved these changes Jan 29, 2026

View reviewed changes

Comment thread crates/eth2util/src/deposit/mod.rs

varex83 requested changes Jan 29, 2026

View reviewed changes

iamquang95 added 4 commits January 30, 2026 14:05

fix: using alloy to deal with Address

38faa3e

fix: use AsRef

cd01bd8

Merge remote-tracking branch 'origin/main' into iamquang95/eth2util/d…

40d2220

…eposit2

fix: resolve merge issue

09eacc7

iamquang95 requested a review from varex83 January 30, 2026 07:51

varex83 approved these changes Jan 30, 2026

View reviewed changes

iamquang95 merged commit ad2d221 into main Jan 30, 2026
4 checks passed

iamquang95 deleted the iamquang95/eth2util/deposit2 branch January 30, 2026 15:51

	pub async fn read_deposit_data_files(cluster_dir: &Path) -> Result<Vec<Vec<DepositData>>> {
	pub async fn read_deposit_data_files(cluster_dir: impl AsRef<Path>) -> Result<Vec<Vec<DepositData>>> {

Conversation

iamquang95 commented Jan 22, 2026

Uh oh!

varex83 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emlautarom1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

varex83 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees