Clean config #33
Clean config #33
Conversation
There was a problem hiding this comment.
Overall, I feel like you might be going in the right track, but I see some rough spots there - in quite some places the readability was impacted considerably, and I feel like you made this way too convoluted from the original goal which was extracting some variables from the Config struct. I see behavior changes, changes in method calls, lots of naming changes, all mixed up. It's simply tough to review it in this form, and the commit history doesn't really tell me anything.
Normally I'd still agree on going through with this idea, even though arguably simply changing the name Config would 'fix' the problem. However, I don't really feel like pushing for an refactor like this one when we have tasks that are way higher priority, especially when it doesn't deliver any actual value. We have to take into account this involves development time, then review time, then fixing time, then re-review time. Also, conflict resolution and so on. It's way too much for something that isn't a feature task.
My opinion is this PR should be put on hold and treated as low priority. If you find any other piece of code that may need refactoring, please create an issue so that we revisit it only when we have more time or when it's relevant to some task - ideally any code change should be delivering value somehow.
|
The following pr on operator set update is based on this one. I made this one earlier in order to make resulting diff smaller and make review easier
Please refer to those in the comments
Those variables don’t belong in config they shall be created and initializaed based on config in appropriate places: clients, signer, txManager in aggregator, logger at the beginning since it used in all scope(could make it a Singleton really).
If you refer to test they will be fixed. What commit history do you expect to see? This pr is simply removing unnecessary things from config.
This is not fixing the problem but masking it. After renaming it doesn't go away. The thing is that it is Config, it is being populated from cli and other entities that are configs. There’re scopes of responsibility for each particular structure, smth it shall and shall not know about. Otherwise we can simply put all of our variables in one struct and pass it around. If you have any reasoning on why logger or client shall be in config I’m open to the discussion.
We focus only on code places that deliver monetary value? Some people will have to read and try to understand that code base I(maybe we) have responsibility to deliver decent code. There’re duplicate code pieces, most of this config belongs in aggregator folder but it’s in here, structures contain unnecessary data, aggregator is aggregating signatures but also its an rpc server at the same time. I don't have an impression that we're behind the schedule on anything to save couple of hours or minutes on work like that.
Sounds like pretty normal workflow for any changes. This is part of our work. If we have technical debt we have to close it. We build our code on top of existing code at the end it will get even harder to change anything. Its like a snowball of architectural decisions that you make. It shall be decent from the start otherwise it turns into nightmare. |
taco-paco
force-pushed
the
refactor/config-clean
branch
from
f591083 to
5b2041f
Compare
taco-paco
force-pushed
the
refactor/config-clean
branch
from
b5193bc to
0691643
Compare
There was a problem hiding this comment.
Some more changes, nothing much!
Keeping with some aggregated conversations:
The following pr on operator set update is based on this one. I made this one earlier in order to make resulting diff smaller and make review easier
This is why I was talking about priorities on future refactors: this PR just forcefully created an artificial dependency. The operator set updates PR does not really depend on this refactor and it's getting even more delayed because of it. Plus, more unrelated changes are being added to this one because it's not yet done and it just was an easier route than another PR.
What I mean by this is the whole workflow got heavily impacted by this PR. If the operator set update PR was done first this would not be an issue. That's why I'd like us to be more careful.
and I feel like you made this way too convoluted from the original goal which was extracting some variables from the Config struct.
Those variables don’t belong in config they shall be created and initializaed based on config in appropriate places: clients, signer, txManager in aggregator, logger at the beginning since it used in all scope(could make it a Singleton really).
see behavior changes, changes in method calls, lots of naming changes, all mixed up. It's simply tough to review it in this form, and the commit history doesn't really tell me anything.
If you refer to test they will be fixed. What commit history do you expect to see? This pr is simply removing unnecessary things from config.
I think anyone seeing the diff would agree this is not "simply removing unnecessary things from config", and that's exactly my point. Even though this was supposed to be a quick local change, the diff was really big - renamings, ctx argument reordering, behaviour changes, error handling changes, and so on.
In order to properly review this, I had to go through everything multiple times, since there were many many potentially dangerous changes simply unrelated to the original goal, and likely most of my comments were in those. In some cases not checking those out would lead to someone down the road directly having to do one more refactor because of this refactor. If I could check change-by-change in the commits, this would be a lot easier, but it's also lots of aggregated changes, and I can't really guide myself by the names - see, e.g. 848897f or 8b56b2c.
All this is manageable, just a pointer for future PRs so we can get it easier for everyone to review.
This is not fixing the problem but masking it. After renaming it doesn't go away. The thing is that it is Config, it is being populated from cli and other entities that are configs. There’re scopes of responsibility for each particular structure, smth it shall and shall not know about. Otherwise we can simply put all of our variables in one struct and pass it around. If you have any reasoning on why logger or client shall be in config I’m open to the discussion.
Of course, that's why I said 'fix', because what was being called Config is actually an environment - and that's why I said it's first of all a naming problem. I actually prefer that whatever it is it's called correctly, if we keep it a config, then your approach is correct.
Though I don't think scopes of responsibility should be above understandability in some cases.
In terms of this, I still think the previous code was a lot more understandable. I'd really encapsulate the client creation as a configClients := NewConfigClients(config) or such and keep this outside the aggregator constructor, especially since this will likely be duplicated on the challenger. Still, either way is manageable and I don't think it's worth the extra work now though.
We focus only on code places that deliver monetary value? Some people will have to read and try to understand that code base I(maybe we) have responsibility to deliver decent code.
See, value != monetary value - and this is a common practice, as it's easy to spend endless time refactoring if there's not a clear goal on what you should achieve with a change, so any change is as valuable. That's the point :)
I don't feel like answering the next part, not sure if anyone would like this kind of reaction, especially as this is a strawman.
There’re duplicate code pieces, most of this config belongs in aggregator folder but it’s in here, structures contain unnecessary data, aggregator is aggregating signatures but also its an rpc server at the same time.
I don't see why any of this issues cannot be formalized as actual issues and be treated smoothly, especially since they're unclear in a few words.
I don't have an impression that we're behind the schedule on anything to save couple of hours or minutes on work like that.
Whenever there's going to be impact on tasks, which is the case here, we can discuss the possibility when scheduling in the usual syncs.
Sounds like pretty normal workflow for any changes. This is part of our work. If we have technical debt we have to close it. We build our code on top of existing code at the end it will get even harder to change anything. Its like a snowball of architectural decisions that you make. It shall be decent from the start otherwise it turns into nightmare.
Of course, what I mean is we need to be careful to do it in an organized way - and this goes all ways, this applies to not only this PR or anyone specifically. Not properly treating priorities and dependencies, as well as code quality, standards, architecture, consistency, etc. just leads to a nightmare on itself.
Reverted logger change due to created issue #41. Hope we can get rid of them in arguments. Want to leave the printing in isolated block still, also couldn't find anything similiar, only one line simple prints. Maybe missed something |
You know that smth went wrong when config contains logger