Move time-related enabling conditions to p2p

[akoptelov]

closes #253

@binier Note that now time passed to enabling condition is slightly differ from what is in the state.time(), but I think this is more consistent, as state.time() might jump back, and time that comes from redux is monotonous. See discussion below.

[binier]

state.time() can't jump back. Why do you think so? In fact it should always be equal to the time that you are passing now to the enabling condition, from what I checked briefly.

Because state.time() also gets the last dispatched action time seen by the reducer and it will match last_action_id in the Store. https://github.com/openmina/redux-rs/blob/0a85a1efb6c817f9714010b801efc7cd882cb947/src/store.rs#L175

[akoptelov]

state.time() can't jump back. Why do you think so? In fact it should always be equal to the time that you are passing now to the enabling condition, from what I checked briefly.

Because state.time() also gets the last dispatched action time seen by the reducer and it will match last_action_id in the Store. https://github.com/openmina/redux-rs/blob/0a85a1efb6c817f9714010b801efc7cd882cb947/src/store.rs#L175

Yes, you're right, I've been looking at the reducer.rs while thinking that this an effect, for some reason...
In this case my replacement should be functionally identical for time used in enabling conditions.

I would also remove that state.last_action completely and instead add last_action_id to the ActionMeta, so it is available for reducer and effects.

[akoptelov]

There are some issues still, probably I screwed up enabling conditions when merged them.

[akoptelov]

@binier Zura, how about that? Having additional time-aware method proved to be error-prone, so I've just added the time parameter to is_enabled.

[tizoc]

Is this required because the substates don't have access to the top-level state time?

What about adding a new is_enabled_with_time method (which redux will call instead of is_enabled) to the trait that is defined as:

pub trait EnablingCondition<State> {
    /// Enabling condition for the Action.
    ///
    /// Checks if the given action is enabled for a given state.
    fn is_enabled(&self, state: &State) -> bool {
        true
    }

    /// Enabling condition for the Action (version with time).
    ///
    /// Checks if the given action is enabled for a given state and timestamp.
    fn is_enabled_with_time(&self, state: &State, _time: crate::Timestamp) -> bool {
        self.is_enabled(state)
    }
}

This allows for the current enabling conditions that do not require this workaround to keep their current definitions.

[akoptelov]

Is this required because the substates don't have access to the top-level state time?

Well, partially. On the other hand, keeping time in state is kind of a workaround.

What about adding a new is_enabled_with_time method (which redux will call instead of is_enabled) to the trait that is defined as:

That was my first idea, but with current impl we need to be super-careful implementing EnablingCondition for the global state. By default it calls is_enabled, and if it is is_enabled_with_time that is used for the substate, then it will be a mistake. With only one method we get rid of such errors.

[akoptelov]

@binier @tizoc If you're fine with this, I'd like to merge it. @binier also this one: o1-labs/redux-rs#1

[tizoc]

@akoptelov right now I cannot come up with a better alternative that solves the issue you mentioned above, so +1 from me