-
Notifications
You must be signed in to change notification settings - Fork 2
Added common code needed for generalized WT #36
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
Conversation
Signed-off-by: Silas Lenihan <[email protected]>
5b7915e to
228fe8f
Compare
d2068cf to
0bd913d
Compare
f3f2844 to
8c4eb17
Compare
… decode their chain-specific configs from it
8c4eb17 to
39f5ab0
Compare
dhaidashenko
left a comment
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.
Looks goode. Feel free to ignore nits
a25a467 to
84df8e2
Compare
84df8e2 to
b46ae76
Compare
3205317 to
ae1e95d
Compare
0874a5a to
5d2fa87
Compare
5d2fa87 to
9e71b84
Compare
fe89137 to
a72a1f1
Compare
a72a1f1 to
510243c
Compare
krebernisak
left a comment
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.
LGTM for the initial move. Nice work!
There's some structural improvements we can do in a follow up:
- Extract common stuff to chainlink-common
- Extract balance framework outside the
/writetargetpath - Extract data-feeds product specifics to a product specific repo
- Inject report decoders so they can be switched by chain/product
| @@ -0,0 +1,78 @@ | |||
| module github.com/smartcontractkit/chainlink-framework/capabilities | |||
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.
Please add a readme with short description and instructions, also point to EVM and Aptos implementations. Thx!
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.
Can also do the same for the balance monitor part, point to Aptos implementation at least.
| cr commontypes.ContractReader | ||
| cw commontypes.ContractWriter |
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.
Wait, why do we still need cr/cw if we're now injecting a TargetStrategy interface to abstract required interactions with the chain. This is now confusing, which interface is used?
I suggest we clean this up and only use a single interface.
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.
We still need CW to check the transaction status later, but good point we no longer need CR in the generalize wt, only in the target strategy.
dhaidashenko
left a comment
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.
Need to address Write Target ID generation
2549b8d to
44c77c2
Compare
44c77c2 to
43a4fc7
Compare
Description
Requires Dependencies
Resolves Dependencies