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

feat(core/header): Consider using a Generic interface instead of a non generic one to define header service and header. #17135

Closed
testinginprod opened this issue Jul 26, 2023 · 7 comments
Assignees

Comments

@testinginprod
Copy link
Contributor

Currently if a module wants to specialise to support a specific runtime it will be required to accept an additional service to properly serve that runtime. This makes the code fail at runtime and also can become hard to maintain in the longterm.

Instead we could define the header.Service  and Header  as generic interfaces. A module can then decide if to specialise for a specific runtime, and be able to use the concrete type which implements the header.

A module which wants to be agnostic to the runtime can instead still use the generic interface.

Full example:

// Header defines a generic header interface.
type Header interface {
	GetHeight() uint64  // GetHeight returns the height of the block
	GetHash() []byte    // GetHash returns the hash of the block header
	GetTime() time.Time // GetTime returns the time of the block
	GetChainID() string // GetChainID returns the chain ID of the chain
	GetAppHash() []byte // GetAppHash used in the current block header
}

// Service defines the interface in which you can get header information,
// given the execution context.
type Service[H Header] interface {
	GetHeader(ctx context.Context) H
}
type CelestiaHeader struct {
	SomethingCelestiaSpecific string
}

// CelestiaHeader implements the Header interface.
func (c CelestiaHeader) GetHeight() uint64 {}
func (c CelestiaHeader) GetHash() []byte {}
func (c CelestiaHeader) GetTime() time.Time {}
func (c CelestiaHeader) GetChainID() string {}
func (c CelestiaHeader) GetAppHash() []byte {}

// Specialises Service to be CelestiaHeader.
func CelestiaModuleFunction(hs Service[CelestiaHeader], ctx context.Context) {
	celestiaHeader := hs.GetHeader(ctx)
	log.Printf("I am using a celestia header specific thing: %s", celestiaHeader.SomethingCelestiaSpecific)
}

// It's generic towards the Header.
func RuntimeAgnosticModule[H Header](hs Service[H], ctx context.Context) {
	agnosticHeader := hs.GetHeader(ctx)
	log.Printf("runtime agnostic so, only use the interface of the header and not the concrete type: %s ", agnosticHeader.GetTime())
}
@github-actions github-actions bot added the needs-triage Issue that needs to be triaged label Jul 26, 2023
@tac0turtle tac0turtle added T:Sprint and removed needs-triage Issue that needs to be triaged labels Jul 26, 2023
@tac0turtle tac0turtle self-assigned this Jul 26, 2023
@tac0turtle
Copy link
Member

tac0turtle commented Jul 26, 2023

i like this approach and think it makes sense, @aaronc do you have thoughts?

i can pick it up

@aaronc
Copy link
Member

aaronc commented Jul 27, 2023

I don't think these generic types would work correctly with depinject dependency resolution and actually adds confusion to the type signatures if you want a non-specific version. Celestial should just provide a specialized celestial header service for custom things and that header could of course implement this header interface.

@Wondertan
Copy link
Contributor

Take a look at the existing go-header lib developed and used by Celestia and Rollkit teams. The core Header interface enables users to define specific header for their needs, similar to what is described in this issue. Its current focus is to enable syncing, storing, and p2p exchange of user-defined blockchain headers. In other words, it implements generic light client functionality over p2p.

According to the interfaces documented on the issue, the only piece go-header misses is something like GetAppHash on the Header interface, which we can effortlessly added. We would add it anyway once we get to state queries over P2P which we could also collaborate on at some point.

A few usages examples:

  • ExtendedHeader in Celestia
    • It's basically a regular Comet header with one additional field defined on our Comet fork.
  • SignedHeader in Rollkit
  • Block in Rollkit
    • You can even manage full blocks with it because Block implement the interface.

Docs on the lib are far from being even acceptable, but the code itself is well-documented(check Syncer for example). Worth mentioning that it works together with go-fraud lib enabling similar generic functionality for user-defined fraud/fault proofs(we have design doc to abstract it to support validity and zk proofs). The end goal is evolve to a lib that generically solves all the p2p problems for chains and rollups.


@aaronc, generic types works well with reflect which is what depinject is based on. Here is an example with FX, which another reflect based DI(btw, I don't get why SDK went with custom solution): https://github.com/celestiaorg/celestia-node/blob/main/nodebuilder/header/module.go#L33-L43

@aaronc
Copy link
Member

aaronc commented Nov 22, 2023

Here is an example with FX, which another reflect based DI(btw, I don't get why SDK went with custom solution)

We did experiments with dig and fx and some other tools. There were quite a lot of things that we wanted to do differently from dig, although it looked like the best inspiration.

@tac0turtle
Copy link
Member

closing this as we decided to go a different path

@Wondertan
Copy link
Contributor

Any links to the new path?

@tac0turtle
Copy link
Member

https://github.com/cosmos/cosmos-sdk/blob/main/docs/rfc/rfc-006-handlers.md#consensus-messages is the approach for now. but we still may explore generic headers in the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🥳 Done
Development

No branches or pull requests

4 participants