input+lnrpc: realign witness types

[emilioziniades]

Fixes #7460

This PR brings the StandardWitnessType enum back in line with the WitnessType enum.

To avoid breaking compatibility with the existing protobuf definition, I added the missing enum values there, and renumbered the enum values in the input package to match.

I wasn't quite sure how to go about testing this. Really you'd want to iterate over all the values of a defined enum. Not sure if this is even possible in Go. So instead I created a map from enum values to strings, and compared the values in that map with the generated protobuf enum values. Admittedly it's not foolproof.

Whilst I was there, I also thought it made sense to refactor the String method to use this new map.

EDIT: I reworked this a little, and the test now uses the String method directly without creating a global map.

EDIT EDIT: went with the explicit map.

[ziggie1984]

I think we need to test the mapping where it is needed in the codebase, because this will not prevent any errors when we are not actually changing the function where the mapping is done. mapping

My recommendation:
Write a function which does the mapping getWitnessType(input.StandardWitnessType) walletrpc.WitnessType and then test this function for all input types and make sure no unknown witness type is returned where its not expected.

Try to use make lint which catches some coding style|formatting bugs

[emilioziniades]

I think we need to test the mapping where it is needed in the codebase, because this will not prevent any errors when we are not actually changing the function where the mapping is done. mapping

My recommendation: Write a function which does the mapping getWitnessType(input.StandardWitnessType) walletrpc.WitnessType and then test this function for all input types and make sure no unknown witness type is returned where its not expected.

Try to use make lint which catches some coding style|formatting bugs

@ziggie1984 thanks for the feedback, I have

• created a getWitnessType function to do the mapping
• made a test to ensure that the mapping is correct
• run make lint and resolved any linting issues

Would you mind taking another look?

[ziggie1984]

Looks really good, left some comments :)

[ziggie1984]

Almost good from my side, had some final nits.

[ziggie1984]

LGTM ⚡️

[guggero]

Thanks for the PR!
Unfortunately we're between a rock and a hard place. We can't change the RPC values to not break older clients. And we can't change the native enum, because that's used for database values.
So we have to rely on the mapping.

[lightninglabs-deploy]

@guggero: review reminder
@emilioziniades, remember to re-request review from reviewers when ready

[guggero]

Looks good, thanks for the update!
Can you rebase so the fixup commits are applied please? Then I'll do the final pass.

[guggero]

nit: logging and error function calls get an exception in the formatting to help distinguish them from actual ("business logic") function calls: https://github.com/lightningnetwork/lnd/blob/master/docs/code_formatting_rules.md#exception-for-log-and-error-message-formatting.

[emilioziniades]

I see, makes sense. I've changed it from

return nil, fmt.Errorf(
        "an error",
)

to

return nil, fmt.Errorf(
        "an error")

[guggero]

Actually, it should even be:

return nil, fmt.Errorf( "an error")

So in this case:

			return nil, fmt.Errorf("unhandled witness type %v for "+
				"input %v", pendingInput.WitnessType,
				pendingInput.OutPoint)

[emilioziniades]

Right. Fixed it.

[emilioziniades]

@guggero I've rebased the fixup commits. The PR is ready for a final pass.

[guggero]

LGTM 🎉