-
Notifications
You must be signed in to change notification settings - Fork 45
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: make bindings, wallet and txManager available #224
Conversation
I will rebase after #203 get merged, the diff will be much more less by then. |
chainio/clients/builder.go
Outdated
Wallet wallet.Wallet | ||
TxManager txmgr.TxManager | ||
AvsRegistryContractBindings *utils.AvsRegistryContractBindings | ||
EigenlayerContractBindings *utils.EigenlayerContractBindings |
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.
@shrimalmadhur any oppositions to returning everything but the kitchen sink in this constructor?
I was thinking we break it up into smaller builders (for eg one for high-level chainio reader/writer/subscribers, and another for low-level interactions), but I don't see any problem with returning everything in here and letting user take whatever they need?
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 me know if you prefer break it, i will adjust my PR, thanks!
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.
IMO,
we should have 3 exported Methods
BuildELClients
- just returns core EL contract Bindings
BuildAVSRegistryClients
- just returns AVS resgistry bindings
BuildAllClients
- returns all
@samlaf how about this and then user can use based on whatever is needed? I don't want someone to force get AVS registry if not needed.
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.
#117 this PR does this. I think we can merge that one first then and rebase this one on top?
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.
yes +1 to that.
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.
Oh really sorry, i didn't notice that there is a PR already! Just make my PR more clean...
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 am totally okay you can go with that one first!
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.
@renlulu no worries, that was an old one... just merged the other PR. Can you rebase 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.
GM, just rebased!
895ac3d
to
ba56877
Compare
ba56877
to
164ab6c
Compare
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.
This works for now. Don't think it solves what I had in mind for #220 so I will leave that open. But at least we have this for now. Thanks :)
Fixes #220 .
What Changed?
Reviewer Checklist