-
Notifications
You must be signed in to change notification settings - Fork 2
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
Create MultiNode Adaptor #7
base: main
Are you sure you want to change the base?
Conversation
988f9b2
to
cfbc9d2
Compare
…tcontractkit/chainlink-framework into BCFR-1071-multinode-adaptor
lifeCycleMu sync.RWMutex | ||
lifeCycleCh chan struct{} | ||
|
||
chainInfoLock sync.RWMutex |
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 does this mutex protect?
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.
Both ChainInfo
structs below it (highestUserObservations
and latestChainInfo
)
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.
Would you think it makes sense to spell it explicitly?
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.
How so?
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.
Right now, it might seem (unless you read the code) that chainInfoLock might only protect latestChainInfo. So, I think spelling out the protected fields explicitly would help 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.
Updated and prefixed both fields with chainInfo
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.
The type is already there (ChainInfo), so I don't think the prefix is necessary - you can just comment that they are both protected by chainInfoLock.
Sorry, the naming seems strange - the Adapter pattern (https://refactoring.guru/design-patterns/adapter) usually helps to convert one interface to another; in this case, however, this functionality seems more like a common part - may be a "SubscriberManager"? |
This component adapts chain-specific client interfaces to the RPCClient interface used by MultiNode. I think the name makes sense here for the use case. I don't feel like "SubscriberManager" really describes what this component is used for. |
Description
Ticket: https://smartcontract-it.atlassian.net/browse/BCFR-1071
Most of the code in the Solana MultiNodeClient can be reused by all chain integrations. This PR extracts all of the chain-agnostic client code through making a generic MultiNodeAdaptor. This will further extract functionality into the MultiNode and reduce new integration cost.
Generic MultiNodeAdaptor:
The the following methods can all be chain-agnostic and extracted:
All other client methods will still be implemented on the chain specific side including: