Updated reducer to accept action instead of & action #806
Conversation
0xMimir
force-pushed
the
feat/update-reducer
branch
from
2af9111 to
63be363
Compare
dkuehr
left a comment
dkuehr
left a comment
There was a problem hiding this comment.
Could you add a description of why this change is needed?
Have you measured this change has a big impact in the amount of stack used?
| let result = p2p::P2pState::reducer( | ||
| Substate::new(state, dispatcher), | ||
| meta.with_action(action), | ||
| meta.with_action(action.clone()), |
There was a problem hiding this comment.
I am thinking that In general, for the remaining of the code, being that we still need to clone the action I would wait until much later to take care of these kind of changes, because I don't think they offer any immediate benefit.
tizoc
left a comment
tizoc
left a comment
There was a problem hiding this comment.
Left a few comments, but LGTM in general. Having said that, I would leave these kind of refactors for later, when we can get an actual benefit from the change, I don't think we get a noticeable improvement from these changes right now (but I may be missing something).
0xMimir
force-pushed
the
feat/update-reducer
branch
from
c9db83c to
53cb88e
Compare
0xMimir
force-pushed
the
feat/update-reducer
branch
from
53cb88e to
74ce6c9
Compare
3 participants
Update reducer to accept action instead of reference to action, this is to make p2p layer ready for full stateful vs effetcful split between actions.
The cloning of an action shouldn't impact the stack size, since the size of
P2pActionis only 184 bytes. In most cases, it needs to be either fully or partially cloned to update the state and/or pass data to new actions.