search

livepeer / protocol

Livepeer protocol
MIT License
152 stars 45 forks source link

bonding: remove duplicate IBondingManager interface #584

Closed red-0ne closed 2 years ago

red-0ne commented 2 years ago

What does this pull request do? Explain your changes. (required) Remove duplicate IBondingManager interface

Specific updates (required)

How did you test each of these updates (required) yarn test yarn deploy

Does this pull request close any open issues? Fixes #560

Checklist:

red-0ne commented 2 years ago

Since the beginning, Interfaces have always been in the same directory as the implementation contract. It is so that the interface of a contract is within the context of the folder name and easier to search. But it also kinda makes sense to group all the interfaces in the same interface directory.

Both make sense IMO. As long as we keep that consistent. We can view it as @kautukkundan addressed it and keep context closer to the implementation or separate abstraction from implementation giving maybe more freedom for both of the realms to evolve.

@RiccardoBiosas We just need to make sure to allocate resources to migrate everything to the directory structure. If that's all good. I'll be good to merge.

RiccardoBiosas commented 2 years ago

I am fine with either but it will be great if it remains consistent. This can be addressed in the same or different PR. Apart from this, changes look good 👍🏼

Yes, good point! The scope of the current issue was narrowed down to just removing the occurrences of duplicate interfaces for the BondingManager contract - which were also declared on the same file as the consumer contracts rather than on their own file. There are similar occurrences in other parts of the codebase, which can potentially lead to cyclic dependency issues. As for where to place the interfaces, IMHO I think they are better off under their own directory interfaces since they are meant for project-wide consumption rather than being tightly related to their implementation. Most of the codebases I'm familiar with follow a similar directory structure/pattern (here, here, here ). Anyway enforcing consistency in that regard should go in a separate issue.

RiccardoBiosas commented 2 years ago

@RiccardoBiosas We just need to make sure to allocate resources to migrate everything to the directory structure. If that's all good. I'll be good to merge.

Sounds good, but please hold off on merging since we are working out how to structure updates to the contract codebase moving forward - where the main branch should just represent the state of the deployed codebase. We can discuss it further offline