Skip to content
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

refactor chainio clients #73

Merged
merged 16 commits into from
Dec 8, 2023
Merged

Conversation

samlaf
Copy link
Collaborator

@samlaf samlaf commented Nov 3, 2023

Fixes #26 .

Managed to get rid of the elChainClient struct by forking abigen to generate interfaces for the structs it generates, and using those interfaces directly in the elWriter/elReader/elSubscriber structs. There's still a bit of clutter (see the few TODOs I left). I think for example we should change the interface of the writers/readers/subscribers to take the geth Call/TxOpts directly, instead of taking a ctx and wrapping it manually in a Call/TxOpts. Also should be reusing the bindings struct to have nicer constructors. I left the elContractBindings struct there for this purpose, since I think we should repurpose it as a constructor (maybe move it to chainio/constructors actually..)

@shrimalmadhur @NimaVaziri please review these changes. If you like them, I'll do the same for the AVS clients.

@samlaf samlaf marked this pull request as draft November 3, 2023 06:03
@samlaf
Copy link
Collaborator Author

samlaf commented Nov 8, 2023

@shrimalmadhur need new review. This is almost ready to go. One thing I'm thinking of doing though is changing the interfaces for all our el/avs readers/writers/subscribers, and have them expose the same interface as the bindings themselves (eg. take a callOpts or transactOpts directly instead of a ctx). Thoughts?

Also did you change the settings of the linter at any point? I didn't change anything with the metrics examples afaik and not it's complaining that EigenMetrics is an unknown identifier (I think because EigenMetrics is in metrics pkg whereas the example test is in metrics_test pkg.. but unsure) EDIT: fixed

linter doesn't accept indirect reference to eigenmetrics.. quite stupid
@samlaf
Copy link
Collaborator Author

samlaf commented Nov 8, 2023

Ok so main question left to complete this refactor @shrimalmadhur @NimaVaziri is what interface we expose for the el/avs reader/writer/subscriber clients. Right now we only expose some ad-hoc methods and add them as we get new needs. For eg
image

2 remaining questions for me are:

  • as I mentioned in the previous comment, I think we should get rid of the ctx arguments and replace them directly with the geth.bind TransactOpts/CallOpts/FilterOpts which wrap a context with extra arguments and give more flexibility to the caller for how they wish the tx to be sent.
  • do we want to keep these APIs minimal, or should we just expose ALL of the underlying binding methods (all the ocntract methods basically). I think there is a problem with doing this in that if we just use struct embeddings there might be method conflicts b/c all the different contracts are initializable for example so all have the same init() method which go doesn't like... but there might be a way (?)

@samlaf samlaf marked this pull request as ready for review December 5, 2023 03:21
@samlaf
Copy link
Collaborator Author

samlaf commented Dec 5, 2023

@shrimalmadhur all changes are in. Ready for final review (hopefully)!

@shrimalmadhur shrimalmadhur merged commit 513ad35 into master Dec 8, 2023
3 checks passed
@shrimalmadhur shrimalmadhur deleted the samlaf/refactor-chainio-clients branch December 8, 2023 23:58
chzyer added a commit to automata-network/eigensdk-go that referenced this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor ChainIO clients structure
2 participants