Add testutils pkg; use TimeFromUnix when building message publications

[johnsaigle]

Use the SDK's new TimeFromUnix timestamp validation function at all call sites, including in test code.

This normalizes timestamp validation to avoid niche overflow scenarios as well as divergences between various types used to represent timestamps in the Go code. It adds an error-handling step but it also makes it so that authors don't need to know the implementation details of how a timestamp is represented. It also allows us to avoid linter warnings by resolving them correctly instead of muting them at every site where a Message Publication is built.

In order to make tests a bit more ergonomic, a
helper test function was added in a new testutils/ package. Going
forward, we can move shared test helpers into this package rather than
re-implementing functions that are needed by many tests (e.g. building
and validating Message Publications).

[johnsaigle]

@djb15 could you re-review?

[djb15]

Thanks for the changes to the logs

[johnsaigle]

In draft until I resolve the dictionary conflict

[djb15]

Still looks good to me

[pleasew8t]

Made two last changes:

• Removed the /scripts/coverage-check/coverage-check binary, which may have been committed by accident. Updated .gitignore to future-proof this.
• Reimported time in the IBC watcher test, since the IBC channel caching PR made some changes that require time.Now().

[bemic]

Looks good to me 👍

[evan-gray]

initial feedback. I think the criticism from accountant watcher applies to other places and I'm concerned overall about the failure behavior change.

[evan-gray]

this matches the whole scripts/coverage-check directory. given there's committed changes there, I assume that was not the intention. were you trying to omit a specific binary?

[johnsaigle]

Can't remember 🤷🏻‍♂️ We can revert this and handle the change in another PR, if needed.

[evan-gray]

why was tokenbridge removed? I would think it was in there on purpose so contributors spelled the compound correctly in code (if they use an editor with cspell support)

[johnsaigle]

I don't remember, we can restore it 👍🏻

[evan-gray]

I actually think this change is bad practice. xfer.Timestamp is already uint32 and this call widens it to number (from a code / input perspective), adding checks that can't possibly be hit and then making coverage worse because there's unreachable code below. Not sure if there's overhead as it might get compiled away, but perhaps a more explicit version like TimeFromUint32 would not have this impact. This may be a criticism of the original TimeFromUnix construction itself.

My point being, the code should enforce that this type never changes from uint32 and should not have an error path that is unreachable and untestable today but the consequences of which are unknown in the future by a potential change which the impacts of which have been covered up.

[johnsaigle]

I agree that in this specific case the check is essentially redundant because the type for xfer.Timestamp is uint32. I do think that it has the benefit of removing the int64() cast which in general is not a great practice because type conversions like this can obscure type errors.

More generally, the generic number is being used to handle existing cases where the type isn't already uint32. I feel like this is better for testing and coverage. The alternative of not doing this in a centralized function is that we have to continually check that each conversion from the raw chain value into the message's Timestamp field is safe. The linting tools we've configured are already flagging these as risky patterns for us, hence all the annotations that "this conversion is safe...".

Ideally each case is and remains safe but using something like TimeForUnix allows us to avoid a footgun. I don't see a clear advantage to writing TimeFromUint32, TimeFromInt64, ..., vs. using a generic -- though we could create a dedicated generic type other than number if we want to further limit the valid range of inputs from the perspective of the callers. I do think that the existing implementation and tests are comprehensive though.

Having a centralized function means we do have potential redundant error paths, but I see this as preferable to the alternatives:

• (status quo) hard casting to a different type when we created MessagePublication, thus hiding errors or drifts in the type
• suppressing the error via timestamp, _ := vaa.TimeFromUnix(x), which would also hide errors

To make it a bit more concrete, this particular case seems safe -- the parseEvent function should return an error if the input doesn't deserialize from JSON into a uint32. That covers the serialization boundary from JSON-to-Go. The error handling here also adds a layer of validation on the boundary of Observation-to-MessagePublication. If someone should erroneously change the JSON decoding or Observation representation (e.g. in a refactor of the Accountant), we lose a guarantee on the Timestamp value. I'd rather make this explicit everywhere rather than rely on checks from one boundary being used transitively to protect another, because that's where drift is most likely to happen.

[evan-gray]

related to the above, as a reader/reviewer, this makes me worry about what happens if this fires even though it never can. I don't think this previously had a failure case an now suddenly it appears to.

[johnsaigle]

We could improve this section by adding a comment to the effect that this is a denfense-in-depth error that we don't expect to happen, given that Observation should carry a uint32 value for Timestamp

[evan-gray]

from LLM review:

someMessage(t) now goes through randomTime(t) → testutils.MustTimeFromUnix(t, …), which can call t.Fatalf. Go's testing package requires Fatal to run on the test's own goroutine, not one started with go func(). It can't actually fail here (Uint32() % 1700000000 is always in range), and go vet doesn't catch it because the call is indirect. Either build the messages before starting the goroutine, or keep a version of randomTime that doesn't take t for this benchmark.

[johnsaigle]

We could build the messages outside of the go routine and iterate over them here instead.

[evan-gray]

TimeFromUnix(b.TimeStamp) runs once per observation inside the loop, but the timestamp is per block. Compute it once