Skip to content

Conversation

@KaloyanTanev
Copy link
Collaborator

@KaloyanTanev KaloyanTanev commented Oct 15, 2025

  1. charon deposit sign: sign partial deposit -> publish to the api
  2. charon deposit fetch: fetch deposits from the api -> aggregate signatures -> save full deposit locally

category: feature
ticket: #3992

@KaloyanTanev KaloyanTanev self-assigned this Oct 15, 2025
@KaloyanTanev KaloyanTanev marked this pull request as draft October 15, 2025 11:36
@codecov
Copy link

codecov bot commented Oct 15, 2025

Codecov Report

❌ Patch coverage is 65.76923% with 89 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.93%. Comparing base (7e84819) to head (6a73eb1).

Files with missing lines Patch % Lines
cmd/depositsign.go 64.35% 19 Missing and 17 partials ⚠️
app/obolapi/deposit.go 61.17% 18 Missing and 15 partials ⚠️
cmd/depositfetch.go 74.50% 6 Missing and 7 partials ⚠️
cmd/cmd.go 0.00% 4 Missing ⚠️
app/obolapi/api.go 0.00% 2 Missing ⚠️
cmd/exit_delete.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4032      +/-   ##
==========================================
+ Coverage   54.81%   54.93%   +0.11%     
==========================================
  Files         242      246       +4     
  Lines       31040    31295     +255     
==========================================
+ Hits        17014    17191     +177     
- Misses      11785    11827      +42     
- Partials     2241     2277      +36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@KaloyanTanev KaloyanTanev marked this pull request as ready for review October 17, 2025 14:56
@KaloyanTanev KaloyanTanev requested review from DiogoSantoss and pinebit and removed request for DiogoSantoss and pinebit October 17, 2025 14:56
@KaloyanTanev KaloyanTanev marked this pull request as draft October 17, 2025 14:59
@KaloyanTanev KaloyanTanev marked this pull request as ready for review October 20, 2025 08:44
Copy link
Collaborator

@pinebit pinebit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for the first pass.

@KaloyanTanev KaloyanTanev force-pushed the kalo/obolapi-deposits branch from f2ef1e0 to 5c1a131 Compare October 23, 2025 12:57
@KaloyanTanev KaloyanTanev force-pushed the kalo/obolapi-deposits branch from 5c1a131 to f134b54 Compare October 24, 2025 09:43
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
15.2% Duplication on New Code (required ≤ 15%)

See analysis details on SonarQube Cloud

}

// do aggregation
fullDeposits := []eth2p0.DepositData{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do make([]eth2p0.DepositData,dr.Amounts)? and then later fullDeposits[i]=... ?

Copy link
Collaborator Author

@KaloyanTanev KaloyanTanev Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we gain from filling the slice with empty objects which we later overwrite over creating an empty one to which we later on amend new objects?

I personally never liked empty objects in slices as it's really error prone and useful only in specific cases.

return errors.Wrap(err, "match local validator key shares with their counterparty in cluster lock")
}

pubkeys := []eth2p0.BLSPubKey{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make([]eth2p0.BLSPubKey,config.ValidatorPublicKeys) and then pubkeys[i]=blsPK

@KaloyanTanev KaloyanTanev requested a review from Copilot October 31, 2025 09:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request introduces deposit signing and fetching functionality for distributed validators, enabling operators to sign and fetch deposit messages through the Obol API. The key purpose is to allow modification of withdrawal addresses after creation but before validator activation.

Key changes:

  • Added new CLI commands for deposit signing (deposit sign) and fetching (deposit fetch)
  • Created deposit handling API endpoints and mock server implementation
  • Refactored common code by extracting shared utilities from exit.go to obolapi.go
  • Renamed ErrNoExit to ErrNoValue to make it reusable for both exits and deposits

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
testutil/obolapimock/obolapi.go New file containing extracted common mock server utilities (testServer, auth middleware, helper functions)
testutil/obolapimock/exit.go Removed duplicated code that was moved to obolapi.go, kept exit-specific logic
testutil/obolapimock/deposit.go New file implementing mock deposit API handlers (submit partial, fetch full)
cmd/deposit.go New file defining shared deposit command configuration and flags
cmd/depositsign.go New file implementing deposit signing command logic
cmd/depositfetch.go New file implementing deposit fetching command logic
cmd/depositsign_internal_test.go New test file for deposit signing functionality
cmd/depositfetch_internal_test.go New test file for deposit fetching functionality
cmd/cmd.go Added deposit commands to the root command
app/obolapi/deposit.go New file implementing client-side deposit API calls
app/obolapi/deposit_model.go New file defining deposit API request/response models
app/obolapi/deposit_test.go New test file for deposit API integration
app/obolapi/exit.go Renamed ErrNoExit to ErrNoValue
app/obolapi/exit_test.go Renamed test functions to be more specific (TestAPIExit, TestAPIExitMissingSig)
app/obolapi/api.go Updated to use renamed ErrNoValue
cmd/exit_fetch.go Updated to use renamed ErrNoValue
cmd/exit_delete.go Updated to use renamed ErrNoValue
cmd/exit_broadcast.go Updated to use renamed ErrNoValue

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants