fix: metrics validator label to use address instead of pk struct formatting

[LePremierHomme]

Closes #1190

I used address instead of the public key hex encoding. Can change that if wanted.

[raulk]

It's unfortunate we have to do this conversion. Two things:

1. We should make record_metrics() fallible by returning a Result<()>. Then we can avoid having panicking code, and the higher level component would log errors in the error event type. @karlem could you drop a link to an example where we use that event type?
2. Would add a TODO so we try to propagate the FVM address in the validator context.

[raulk]

@LePremierHomme nit: the "closing" keyword is not supported by GitHub to link the issue, but "closes", "close", "closed" are. https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

[LePremierHomme]

1. We should make record_metrics() fallible by returning a Result<()>. Then we can avoid having packing code, and the higher level component would log errors in the error event type. @karlem could you drop a link to an example where we use that event type?

Not sure I understand the purpose of having it fallible. To propagate encoding errors?

2. Would add a TODO so we try to propagate the FVM address in the validator context.

The public key is what's used as the validator key for comparisons. I guess it would be better to just derive Address as a view, or fix it as an additional field in the context. It seems to be currently needed just for the metrics, so I could also just change CheckpointSigned to contain the address instead of the public key.

[karlem]

We should make record_metrics() fallible by returning a Result<()>. This way, we can avoid having packing code, and the higher-level component would log errors using the error event type. @karlem, could you provide a link to an example where we use this event type?

In general, I agree—failing to record a metric should not cause a panic that shuts down the whole node. Here's an example of how to emit a tracing error:

ipc/fendermint/vm/interpreter/src/fvm/exec.rs

Line 200 in 409c138

emit(TracingError {

.

However, given point 2:

Would add a TODO so we try to propagate the FVM address in the validator context.

I would suggest we include this change in the current PR. It’s a simple modification—see here:

ipc/fendermint/app/src/cmd/run.rs

Line 128 in 409c138

ValidatorContext::new(sk, broadcaster)

.

By including the address in the event directly, we can skip the conversion altogether and eliminate the need to make record_metrics() fallible. At least for now!

[karlem]

@LePremierHomme , the address is already being converted in run.rs. I suggest including it in the ValidatorContext and then using it directly in the event.

[raulk]

SGTM!

[raulk]

Thank you, @karlem! 🙏

[LePremierHomme]

Changed ValidatorContext to hold the to_address conversion made during app start.

As for making record_metrics() fallible: none of the current implementations contain fallible code, and I think it’s unlikely that they will.
Any thoughts on that? @karlem @raulk

[karlem]

@LePremierHomme I think it's fine like this now. Since we do not cause panic in the record_metrics.