chore: Audit Textual any, coins, message

[amaury1093]

Description

ref: #15549

Internal audit of the following pieces:

• Any
  • in refactor(textual): Remove msg header screen #15208, we forgot to update the spec. Updating here.
• Coins
  • add test case for "zero"
• Messages
  • Update spec to remove special sdk.Msg handling
• types.go and handler.go
  • add some godocs

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[amaury1093]

If the first condition isMsgRenderer is true, then the second condition also must be true, or else it's an implementation error upstream (e.g. in the protobuf values passed into Textual), and the user should be aware of it.

I propose to return an error here.

[JimLarson]

This puts some constraints on the future evolution of messageValueRenderer. E.g. suppose we decide to allow localization and render "%s objeto" when the account's locale is "PT". With the previous code we just skip the header suppression, but now it becomes an error.

Of course, the current code is sensitive to the future evolution of messageValueRenderer as well. We should at least add a cross-reference to this header recognition code from messageValueRenderer.header() so that we can update this code if we ever modify the header.

[facundomedica]

I don't know if I fully understand your point @JimLarson it seems like if we don't return an error here, it should fail at signature verification right?

I think returning an error is fine, as long as we check against msgRenderer.header() (and stop using fmt.Sprintf; I have that in a separate PR).

To evolve the message renderer we could modify the header() to do stuff based on the message, or something like that.

[facundomedica]

@JimLarson I'll merge this, we can re-discuss this in my follow up pr

[JimLarson]

Following up on this - since we're always adding the expected header back in Parse(), it wouldn't round-trip correctly if MessageValueRenderer didn't add the expected header, so it's probably the best to do as this code does - throw an error if you don't see the expected header. I withdraw my suggestion.

[JimLarson]

Looks good with some minor changes.

[JimLarson]

This puts some constraints on the future evolution of messageValueRenderer. E.g. suppose we decide to allow localization and render "%s objeto" when the account's locale is "PT". With the previous code we just skip the header suppression, but now it becomes an error.

Of course, the current code is sensitive to the future evolution of messageValueRenderer as well. We should at least add a cross-reference to this header recognition code from messageValueRenderer.header() so that we can update this code if we ever modify the header.