Hide ibc::Timestamp::now() behind clock feature flag

[soareschen]

Closes: #1612

Description

This PR enables the ibc::Timestamp::now() method only when ibc/clock or ibc/std feature is enabled. This allows ibc to be used in no_std environment, where the system clock is not available.

---

PR author checklist:

• Added changelog entry, using unclog.
• Added tests: integration (for Hermes) or unit/mock tests (for modules).
• Linked to GitHub issue.
• Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

• Reviewed Files changed in the GitHub PR explorer.
• Manually tested (in case integration/unit/mock tests are absent).

[hu55a1n1]

Wondering if it would be better to add a fn host_timestamp(&self) -> Timestamp; to the ClientReader trait (just like ChannelReader::host_timestamp()) and call that here?

[soareschen]

I think ultimately getting the system time is a side effect, and unrestricted use of it within nested function calls make the code difficult to reason. Ideally I think it is best to keep the application logic pure by having the current time passed as explicit parameter from the outer layer. This would also make the functions easier to test, as compared to the current behavior of the mock code.

I think the mock code using now() everywhere is a problem. And in this PR we can see that the directly use of now() is almost always from the mock or test code. But I also think there are many more problems with the mock code, including whether to separate them into a separate crate and keep the ibc crate pure. So I think they are outside of the scope of this PR.

[hu55a1n1]

I think ultimately getting the system time is a side effect, and unrestricted use of it within nested function calls make the code difficult to reason.

I think this is a valid concern, but the process() function isn't pure even now since it calls ClientReader methods that are usually reading from a DB. IBC handler logic is inherently impure but (mostly) deterministic. Although I agree that it would make sense to separate pure logic for better testability, I don't think it is appropriate to add an additional parameter (which is only used in a couple of code-paths) to an entry-point public API, such as ics26_routing::deliver().
Possible alternatives (not to say that we implement this in this PR) -

1. Separate the pure handler logic into an inner function.
2. Set the timestamp in the higher level ics26_routing::dispatch() function where other side-effects are applied (i.e. the *Keeper methods are called to persist the results).

[soareschen]

IBC handler logic is inherently impure but (mostly) deterministic

Yes, I think that is the important point. The interface for ClientReader looks deterministic to me, and it should probably be referential transparent which is almost as good as pure even if it involves IO. On the other hand, getting the system time is non-deterministic, so we should push that to the edge of the system to keep most logic deterministic.

It is also bad to have multiple calls to now() inside a nested function call, as each call will return slightly different value. Unless we are explicitly measuring time, it is simpler to assume that the current time value remains constant within a function call.

One issue I see with the current discussion is that I do not see much of the refactoring being used in the ibc-relayer module, and the changes over there is minimal. That makes it difficult to assess whether the new design brings any negative impact. From the use of now() in the mock context, I think passing the current time explicitly makes the logic clearer and align with the goal of making things easier to test with mocks.

I quickly skimmed though basecoin-rs, and it also looks like the PR shouldn't have much impact on the current design of Basecoin? Do let me know if there is any impact that I should be aware of.

I am now also looking into whether if the new design in this PR can help fixing #1687, which is a demonstration of why accessing time implicitly makes it difficult to mock over long period of time.

[hu55a1n1]

Thanks for the insightful comments, Soares. This discussion got me thinking about the non-determinism of the way we get time from the host and we found a bug in our implementation -> https://github.com/informalsystems/ibc-rs/issues/1770. We should've used the host's latest on-chain block's header as our time source instead of getting it from the user. I opened #1771 to fix this. With the fix, getting the timestamp provides the same deterministic guarantees as most other ClientReader methods because we're now getting the time from the data store.
PS: the new strategy for getting time from the latest block header might also help simplify mocking time.

[soareschen]

I tried to mock the time and make the tests easier to test without a global clock, but unfortunately there are too many of them and I ultimately got stuck in mocking the time inside the ChainHandle. So I have reverted all other changes and keep this PR minimal.

[mzabaluev]

Looks good to me. Some new lints have emerged, but these can be fixed in another PR.

[romac]

Looks great! Just needs a changelog entry before we can merge.

[soareschen]

Added the changelog now.

Some new lints have emerged, but these can be fixed in another PR.

I ran cargo clippy with the latest version of Rust, and no new lint error appears.