chore: abstract randomness committer #515

Lazar955 · 2025-07-12T10:13:45Z

Wait for #512 to be merged.

closes #513

bot generated:

This pull request refactors the randomness commitment logic in the finality-provider by extracting it into a new, dedicated component (RandomnessCommitter), improving modularity and testability. The main changes include:

Introduction of a new interface types.RandomnessCommitter and its implementation DefaultRandomnessCommitter (service/rand_committ.go), encapsulating the logic for determining when and how to commit public randomness.
Refactoring FinalityProviderInstance and FinalityProviderApp to accept and use a RandomnessCommitter instead of handling randomness commitments internally.
Movement of functions related to randomness commitment and commitment eligibility from FinalityProviderInstance to DefaultRandomnessCommitter.
Update of test helpers and test managers to construct and inject the new committer in tests and integration tests.
Minor renaming for clarity: pubRandState → PubRandState, newPubRandState → NewPubRandState.
Removal of duplicated or obsolete code related to randomness commitment from fp_instance.go, with all logic now handled in the committer.
All calls to commit, check, or retrieve randomness commitments now go through the RandomnessCommitter interface.
Internal wiring and parameter lists updated throughout the codebase to accommodate the new committer dependency.

Overall, this PR decouples the randomness commitment process from the core finality provider logic, laying the groundwork for easier maintenance, extension, and potential alternative commitment strategies in the future.

…to lazar/consumer-interface-ref

Copilot

Pull Request Overview

This PR extracts the public randomness commitment logic into a new RandomnessCommitter component to improve modularity and testability, and updates all callers and tests to use the new interface with context-aware method signatures.

Introduce RandomnessCommitter interface and DefaultRandomnessCommitter implementation.
Refactor FinalityProviderApp and FinalityProviderInstance to accept and use RandomnessCommitter.
Update test utilities, mocks, and integration tests to pass a context.Context to all consumer controller calls.

Reviewed Changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
types/expected_rand_committer.go	Added `RandomnessCommitter` interface definition.
finality-provider/service/rand_committ.go	New `DefaultRandomnessCommitter` implementation.
finality-provider/service/fp_instance.go	Refactored instance to use the committer instead of inline randomness logic.
testutil/utils.go	Updated mock imports and expectation signatures for context parameters.
go.mod	Added direct dependency on `go.uber.org/mock` and cleaned up the indirect entry.

Comments suppressed due to low confidence (2)

types/expected_rand_committer.go:5

Import alias types conflicts with the local package name. Rename the imported package, for example to babylonTypes, to avoid shadowing the current package.

	"github.com/babylonlabs-io/babylon/v3/types"

finality-provider/service/rand_committ.go:1

There are no dedicated unit tests for DefaultRandomnessCommitter. Add tests covering its ShouldCommit and Commit methods to verify all logic branches.

package service

Copilot · 2025-07-14T06:37:33Z

finality-provider/service/rand_committ.go

@@ -0,0 +1,268 @@
+package service


[nitpick] Filename rand_committ.go appears to have a typo. Consider renaming it to rand_committer.go or rand_commit.go for clarity.

Copilot · 2025-07-14T06:37:34Z

finality-provider/service/fp_instance.go


 	fp.wg.Add(2)
 	go fp.finalitySigSubmissionLoop()
-	go fp.randomnessCommitmentLoop()
+	// todo(lazar): will fix ctx in next PRs
+	go fp.randomnessCommitmentLoop(context.Background())


[nitpick] The randomness commitment loop uses context.Background() which cannot be cancelled. Consider passing a cancellable context to support graceful shutdown.

read the todo above it bot 🤷‍♂️

types/expected_rand_committer.go

clientcontroller/api/interface.go

finality-provider/service/app.go

SebastianElvis

Overall LGTM 👍 Some final comments on the abstraction

SebastianElvis · 2025-07-15T06:20:42Z

bsn/rollup-finality-provider/clientcontroller/consumer.go

 	// TODO: implement slashed or jailed feature in rollup BSN
 	return false, false, nil
 }



Suggested change

SebastianElvis · 2025-07-15T06:23:23Z

finality-provider/service/fp_instance.go

 }

 // CommitPubRand commits a list of randomness from given start height
 func (fp *FinalityProviderInstance) CommitPubRand(ctx context.Context, startHeight uint64) (*types.TxResponse, error) {


Do we need this function given that it's 1 line? Same q for GetLastCommittedHeight

SebastianElvis · 2025-07-15T06:24:37Z

finality-provider/service/pub_rand_store_adapter.go

Since we are abstracting out the pub rand commit utilities, do you think this KV store should be made entirely internal for DefaultRandomnessCommitter?

SebastianElvis · 2025-07-15T06:25:52Z

finality-provider/service/fp_instance.go


 	fpState      *fpState
-	pubRandState *pubRandState
+	pubRandState *PubRandState


Similar to the other comment, do you think we should get rid of this pubRandState field from the FinalityProviderInstance, and instead only rely on rndCommitter for pub rand operations?

I actually like the idea of RandomnessCommitter owning the storage 👍 thoug this will be probably ad Close() function to the interface to close the storage on committer level

This can be done in separate pr though.

Make sense, tho we'll have to expose more methods in the interface, but I think that's okay. Will create an issue for this and do in separate PR, don't want to drag this along

SebastianElvis · 2025-07-15T06:26:28Z

finality-provider/service/app.go

-	fpInsMu     sync.RWMutex // Protects fpIns
-	fpIns       *FinalityProviderInstance
-	eotsManager eotsmanager.EOTSManager
+	rndCommitter types.RandomnessCommitter


Same comment for L41, is it good idea to get rid of pubRandStore?

KonradStaniec

Looks good, it is definitly step in good direction 👍

KonradStaniec · 2025-07-15T07:44:41Z

finality-provider/service/fp_instance.go


 	fpState      *fpState
-	pubRandState *pubRandState
+	pubRandState *PubRandState


I actually like the idea of RandomnessCommitter owning the storage 👍 thoug this will be probably ad Close() function to the interface to close the storage on committer level

This can be done in separate pr though.

KonradStaniec · 2025-07-15T07:46:49Z

types/expected_rand_committer.go

+	// GetLastCommittedHeight retrieves the last height at which randomness was committed.
+	GetLastCommittedHeight(ctx context.Context) (uint64, error)
+
+	SetBtcPk(btcPk *types.BIP340PubKey)


SetBtcPk and SetChainID seem like they do not belong here 🤔 maybe they can be made part of the config passed to particular implementations ?

Yep, definitely feels ugly. One option would be to pass them as params to GetLastCommittedHeight. The reason we can't pass them to the ctor of the struct implementing the interface is because we don't know them at that point of time (in case where FPI hasn't been registered yet)

Lazar955 added 13 commits July 9, 2025 16:24

wip interface refactor

6b4a422

Merge branch 'main' of github.com:babylonlabs-io/finality-provider in…

a4109e0

…to lazar/consumer-interface-ref

wip: modified all interfaces, changed mock tool;

e82e3aa

fix all interfaces impl, change types, lint fixes

2b29896

fix double sign e2e

56b523f

house keeping

d64e897

big boi merge fix conflicts

6e1b3b6

cleanup

f3aca18

✍️ changelog

075660a

pr comments, ctor for req structs

01681c8

abstract rnd committer

0d6b645

✍️ changelog

c3c9d3f

lint fix

ad6147b

Base automatically changed from lazar/consumer-interface-ref to main July 14, 2025 06:34

Lazar955 marked this pull request as ready for review July 14, 2025 06:35

Lazar955 requested review from KonradStaniec, SebastianElvis and Copilot July 14, 2025 06:35

Copilot AI reviewed Jul 14, 2025

View reviewed changes

SebastianElvis reviewed Jul 14, 2025

View reviewed changes

types/expected_rand_committer.go Show resolved Hide resolved

clientcontroller/api/interface.go Show resolved Hide resolved

finality-provider/service/app.go Show resolved Hide resolved

big boi merge fix conflicts

f09266d

SebastianElvis reviewed Jul 15, 2025

View reviewed changes

KonradStaniec approved these changes Jul 15, 2025

View reviewed changes

pr comments

4d73238

Lazar955 merged commit 734d9a2 into main Jul 15, 2025
18 checks passed

Lazar955 deleted the lazar/abstract-rand-commit branch July 15, 2025 08:22

chore: abstract randomness committer #515

chore: abstract randomness committer #515

Uh oh!

Conversation

Lazar955 commented Jul 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

bot generated:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SebastianElvis 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!

SebastianElvis Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

KonradStaniec 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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Lazar955 commented Jul 12, 2025 •

edited

Loading

SebastianElvis Jul 15, 2025 •

edited

Loading