-
Notifications
You must be signed in to change notification settings - Fork 208
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
Proposal: Generic subscription options while adding contract listener #1396
base: main
Are you sure you want to change the base?
Conversation
- Options can be used to pass options for automatically created subscription Signed-off-by: Niklas Emmelius <[email protected]>
Signed-off-by: Niklas Emmelius <[email protected]>
Thanks for opening this! This seems like an interesting idea and I or one of other maintainers will review this and provide some feedback as soon as we can, though most of them are a bit busier than usual at the moment. |
Ah, it looks like the linter is complaining because a file was updated by the copyright year in the header wasn't also updated. Sorry, it's a silly linter rule.
|
@@ -44,7 +44,8 @@ type ContractListenerWithStatus struct { | |||
Status interface{} `ffstruct:"ContractListenerWithStatus" json:"status,omitempty" ffexcludeinput:"true"` | |||
} | |||
type ContractListenerOptions struct { | |||
FirstEvent string `ffstruct:"ContractListenerOptions" json:"firstEvent,omitempty"` | |||
FirstEvent string `ffstruct:"ContractListenerOptions" json:"firstEvent,omitempty"` | |||
SubscriptionOptions *fftypes.JSONAny `ffstruct:"ContractListenerOptions" json:"subscriptionOptions,omitempty"` |
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.
I'm struggling with what this field should be named. For context, in the older ETHConnect codebase we used to call these "subscriptions". In the newer FFTM API (which EVMConnect is based on), we now refer to these as "listeners" to help distinguish them from FireFly Subscriptions (which are a higher level object). So this field actually ends up maping to Listener.Options
https://github.com/hyperledger/firefly-transaction-manager/blob/fe5ace887c9f541c3053fd31d937d46ce6adc3c9/pkg/apitypes/api_types.go#L212
But calling this field ContractListenerOptions.ListenerOptions
seems a bit redundant 🤔
@awrichar or @peterbroadhurst Do you have any suggestions 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.
@nguyer - sorry I missed this before.
I think the consistent pattern here with other things, would be that the options
are passed through to EVMConnect. If you look at invoke
APIs, or the tokens - that's what we do I believe.
Closing and re-opening to get the build to run again |
@niklas-emmelius-mittelstand-ai Sorry it took so long to get feedback on this one. If you want to rename |
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.
You'll also want to run make
locally to make sure the build is passing. Thanks!
Generic subscription options while adding contract listener
Generic options for the automatically generated subscription can be passed in listener options
Intention
A privacyGroupId could be passed to Ethconnect and calls to priv_getLogs etc. would be possible.
Every plugin would be able to listen for the subscriptionOptions necessary for its subscription configuration.
The introduced changes do not break existing functionality.