Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Settlement layer redundent abstraction layer #629

Closed
mtsitrin opened this issue Apr 4, 2024 · 2 comments · Fixed by #873
Closed

refactor: Settlement layer redundent abstraction layer #629

mtsitrin opened this issue Apr 4, 2024 · 2 comments · Fixed by #873
Assignees

Comments

@mtsitrin
Copy link
Contributor

mtsitrin commented Apr 4, 2024

for the settlement module, we have 2 interfaces:


// LayerI defines generic interface for Settlement layer interaction.
type LayerI interface {

	// Init is called once for the client initialization
	Init(config Config, pubsub *pubsub.Server, logger log.Logger, options ...Option) error

	// Start is called once, after Init. It's implementation should start the client service.
	Start() error

	// Stop is called once, after Start. It should stop the client service.
	Stop() error

	// SubmitBatch tries submiting the batch in an async way to the settlement layer. This should create a transaction which (potentially)
	// triggers a state transition in the settlement layer. Events are emitted on success or failure.
	SubmitBatch(batch *types.Batch, daClient da.Client, daResult *da.ResultSubmitBatch) error

	// RetrieveBatch Gets the batch which contains the given height. Empty height returns the latest batch.
	RetrieveBatch(stateIndex ...uint64) (*ResultRetrieveBatch, error)

	// GetSequencersList returns the list of the sequencers for this chain.
	GetSequencersList() []*types.Sequencer

	// GetProposer returns the current proposer for this chain.
	GetProposer() *types.Sequencer
}

// HubClient is an helper interface for a more granualr interaction with the hub.
// Implementing a new settlement layer client basically requires embedding the base client
// and implementing the helper interfaces.
type HubClient interface {
	Start() error
	Stop() error
	PostBatch(batch *types.Batch, daClient da.Client, daResult *da.ResultSubmitBatch) error
	GetLatestBatch(rollappID string) (*ResultRetrieveBatch, error)
	GetBatchAtIndex(rollappID string, index uint64) (*ResultRetrieveBatch, error)
	GetSequencers(rollappID string) ([]*types.Sequencer, error)
}

There is redundancy between them.
design-wise, [mock, grpc, dymension] should implement only the Layer interface
The hub client should be used by dymension if needed

@mtsitrin mtsitrin mentioned this issue Apr 6, 2024
12 tasks
@danwt
Copy link
Contributor

danwt commented Apr 8, 2024

I haven't looked at this closely, but generally it's good to have interfaces on the consumer side of any logic.

For example, instead of having A expose an interface IA, and having B,C use IA it's better to have interfaces IB and IC, which may be a subset of IA, and having A implement them both

Of course, there are situations where this principle can be ignored (golang Stringer interface)

@mtsitrin mtsitrin self-assigned this Apr 18, 2024
@omritoptix
Copy link
Contributor

The current hierarchy was written in order to have a base client which fulfills the LayerI interface so all common logic between clients can be extracted to the base.

Than each new settlement layer client should only implement the relevant parts from HubClient interface and doesn't need to implement the common logic (which resides in base).

If we make it the same interface I see the following downsides:

  1. all the common logic that was extracted to the base would be have to be re-written by each client.
  2. would be harder to unit test client specific logic (i.e hubClient logic)

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 a pull request may close this issue.

3 participants