-
Notifications
You must be signed in to change notification settings - Fork 3
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
Blockchain events subscription #77
Conversation
37a9944
to
67a3abc
Compare
ec30fe8
to
c0f64b7
Compare
blockchain/pallets/ddcclusters.go
Outdated
type DdcClustersApi interface { | ||
GetClustersNodes(clusterId ClusterId) ([]NodePubKey, error) | ||
SubscribeNewClusterCreated() (*NewEventSubscription[EventDdcClustersClusterCreated], error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need separate method (and duplicated implementation) for each event type? can't we have single method in BlockchainClient that allows client to specify even type he is interesting in?
e.g. blockchainClient.subscribe(ClusterNodeAdded) and receive clusterNodeAdded event channel ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's consider what blockchainClient.Subscribe
implementation may look like.
It may accept event type instance as a parameter which should be an empty interface to allow arguments of different types.
func (client *Client) Subscribe(eventT any) ???
We may make a type assertion in the body and return an error if the user provides an unknown type.
func (client *Client) Subscribe(eventT any) (???, error) {
switch eventT.(type) {
case EventDdcClustersClusterNodeAdded:
return ???, nil
// snip
default:
return ???, errors.New("uknown event type")
}
}
Return type should be an empty interface too because we want one function to subscribe to all event types.
func (client *Client) Subscribe(eventT any) (any, error) {
switch eventT.(type) {
case EventDdcClustersClusterNodeAdded:
return make(chan EventDdcClustersClusterNodeAdded), nil
// snip
default:
return nil, errors.New("uknown event type")
}
}
This way the client will need to make a type assertion of the return value.
ch, _ := client.Subscribe(EventDdcClustersClusterNodeAdded{})
ch := ch.(chan EventDdcClustersClusterNodeAdded)
Also no hints will be available for the client API for the user to check what subscriptions available. But the user may check available event types.
Do you see a way to avoid client-side type assertion using this approach? If no, the existing API looks better to me. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as for me it could be as simple as:
type Client struct {
// other fields...
eventListeners []EventListener
}
type EventListener func(events *pallets.Events)
func (c *Client) StartEventsListening() {
// other code
for _, eventListener := range c.eventListeners {
eventListener(events)
}
// other code
}
func main() {
blockchainClient, _ := NewClient("")
clusterUpdatedListener := func(events *pallets.Events) {
for _, nodeAdded := range events.DdcClusters_ClusterNodeAdded {
println("node added " + nodeAdded.NodePubKey.AsStoragePubKey.ToHexString())
}
for _, nodeDeleted := range events.DdcClusters_ClusterNodeDeleted {
println("node deleted " + nodeDeleted.NodePubKey.AsStoragePubKey.ToHexString())
}
}
blockchainClient.RegisterEventListener(clusterUpdatedListener)
}
so lib client (storage node) can add listeners that listen for whole events
object and uses events which are needed. E.g. routing table can have listener that uses both DdcClusters_ClusterNodeAdded and DdcClusters_ClusterNodeDeleted events
in such implementation both SDK and client code could be simplified.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Callbacks look good, let me implement it.
blockchain/client.go
Outdated
errCh <- fmt.Errorf("events listener: %w", err) | ||
} | ||
|
||
for _, module := range c.subs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm. do i correctly understand that we are replicating events to all subs here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All block events are stored in one large structure (substrate API library recommended method) and here we pass it by pointer to each pallet. For a smaller structure best practices tell us to copy it while read only access is expected, but this one contains all events of the block and it can be too large to copy.
Also we can have no events filtering by pallet here because pallets will only pick events then need.
}, nil | ||
} | ||
|
||
func (c *Client) StartEventsListening() (func(), <-chan error, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what would happen in case this is called twice? shouldn't we rewrite it somehow so the client can be subscribed to changes only once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Now we can freely call it twice and concurrently. It will ignore a duplicate attempt to start events listening (until its stopped) and just returns an existing stopper function and errors channel.
Client API extended with a function to start events listening and subscribe a callback on them. Manually tested locally.