chore: fix textual tests #18849

tac0turtle · 2023-12-21T00:31:14Z

Description

this pr fixes tests around sign mode textual and recent governance changes. Mainly sign mode textual signing for the same message is different now due to cbor encoding

cc @julienrbrt

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
confirmed ! in 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
reviewed "Files changed" and left comments if necessary
included the necessary unit and integration tests
added a changelog entry to CHANGELOG.md
updated the relevant documentation or specification, including comments for documenting Go code
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 all author checklist items have been addressed
reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

julienrbrt · 2023-12-21T00:34:34Z

x/tx/signing/textual/internal/testdata/tx.json

In theory it should be able to support the previous string as well in the tx 🤔. This is why we had a proto enum alias.
Makes sense for the display however.

this was failing locally for me. I think its specific to sign mode textual, being that this will be our main signing protocol we should probably better understand if changing field names breaks signing as well. @facundomedica do you know if this is true?

I can't reproduce, and if I apply the changes from this PR it fails with:

proto: (line 19:17): invalid value for enum type: "VOTE_OPTION_ONE" Test: TestTxJSONTestcases/a_bit_of_everything

I'm still digging into this to see what's going on, but yes if the string representation of a value changes, the CBOR representation will change

julienrbrt · 2023-12-21T11:54:16Z

So, I have checked-out main, and I am unable to reproduce the failing test on main.
Can you point me to it?

cd x/tx
go test ./...
?       cosmossdk.io/x/tx/internal/testpb       [no test files]
?       cosmossdk.io/x/tx/signing/aminojson/internal/aminojsonpb        [no test files]
?       cosmossdk.io/x/tx/signing/aminojson/internal/testpb     [no test files]
?       cosmossdk.io/x/tx/signing/std   [no test files]
?       cosmossdk.io/x/tx/signing/testutil      [no test files]
?       cosmossdk.io/x/tx/signing/textual/internal/textualpb    [no test files]
ok      cosmossdk.io/x/tx/decode        1.110s
ok      cosmossdk.io/x/tx/signing       0.012s
ok      cosmossdk.io/x/tx/signing/aminojson     0.624s
ok      cosmossdk.io/x/tx/signing/direct        0.012s
ok      cosmossdk.io/x/tx/signing/directaux     0.012s
ok      cosmossdk.io/x/tx/signing/textual       0.044s
ok      cosmossdk.io/x/tx/signing/textual/internal/cbor 0.005s

cannot reproduce.

tac0turtle · 2023-12-21T13:40:50Z

    --- FAIL: TestTxJSONTestcases/a_bit_of_everything (0.00s)
        tx_test.go:84: 
                Error Trace:    /Users/markobaricevic/code/CoTend/cosmos-sdk/x/tx/signing/textual/tx_test.go:84
                Error:          Not equal: 

-- FAIL: TestTxJSONTestcases (0.00s)
    --- FAIL: TestTxJSONTestcases/long_text_in_nested_value (0.00s)
        tx_test.go:84: 
                Error Trace:    /Users/markobaricevic/code/CoTend/cosmos-sdk/x/tx/signing/textual/tx_test.go:84
                Error:          Not equal: 


--- FAIL: TestE2EJSONTestcases (0.00s)
    --- FAIL: TestE2EJSONTestcases/a_bit_of_everything (0.00s)
        e2e_test.go:66: 
                Error Trace:    /Users/markobaricevic/code/CoTend/cosmos-sdk/x/tx/signing/textual/e2e_test.go:66
                Error:          Not equal:

It could be due to the go.work file i have,

all with the same error

      Diff:
                                --- Expected
                                +++ Actual
                                @@ -111,3 +111,3 @@
                                   Title: (string) (len=6) "Option",
                                -  Content: (string) (len=15) "VOTE_OPTION_YES",
                                +  Content: (string) (len=15) "VOTE_OPTION_ONE",
                                   Indent: (int) 2,
                Test:           TestE2EJSONTestcases/a_bit_of_everything

julienrbrt · 2023-12-21T14:55:17Z

    --- FAIL: TestTxJSONTestcases/a_bit_of_everything (0.00s)
        tx_test.go:84: 
                Error Trace:    /Users/markobaricevic/code/CoTend/cosmos-sdk/x/tx/signing/textual/tx_test.go:84
                Error:          Not equal: 

-- FAIL: TestTxJSONTestcases (0.00s)
    --- FAIL: TestTxJSONTestcases/long_text_in_nested_value (0.00s)
        tx_test.go:84: 
                Error Trace:    /Users/markobaricevic/code/CoTend/cosmos-sdk/x/tx/signing/textual/tx_test.go:84
                Error:          Not equal: 


--- FAIL: TestE2EJSONTestcases (0.00s)
    --- FAIL: TestE2EJSONTestcases/a_bit_of_everything (0.00s)
        e2e_test.go:66: 
                Error Trace:    /Users/markobaricevic/code/CoTend/cosmos-sdk/x/tx/signing/textual/e2e_test.go:66
                Error:          Not equal:

It could be due to the go.work file i have,

all with the same error

      Diff:
                                --- Expected
                                +++ Actual
                                @@ -111,3 +111,3 @@
                                   Title: (string) (len=6) "Option",
                                -  Content: (string) (len=15) "VOTE_OPTION_YES",
                                +  Content: (string) (len=15) "VOTE_OPTION_ONE",
                                   Indent: (int) 2,
                Test:           TestE2EJSONTestcases/a_bit_of_everything

Right, then it is using the latest api module with the latest gov proto changes.
Given that we tag x/tx from main, I don't think we can merge this given we use the latest available api for v0.50.
@facundomedica, why doesn't it support proto enum aliases? Should we maybe swap which one is the alias, and which isn't to keep backward compatibility? I am able with sign mode direct to sign with both and it both works.

facundomedica · 2023-12-22T09:47:22Z

The issue is that textual uses the enum's name in the sign bytes, and this enum name is get using protoreflect's EnumValueDescriptors.ByNumber, which If multiple have the same number, the first one defined is returned..

The proto representation doesn't include the name of the value, only the enum itself which is not enough for textual to know which string it should use for display and sign.

Alternatives:

We prohibit the usage of aliases
We modify textual to display all the possible enum string representations, like so:

"VOTE_OPTION_ONE, VOTE_OPTION_YES"

IMO, option 2 would be fine but from a user perspective it will be very confusing. It will only take a gov proposal that uses option one for NO instead of YES. And maybe even exploitable?

julienrbrt · 2023-12-22T10:15:35Z

We actually only want to display it as option when it's a multiple choice proposal.

So 1. We could swap back the alias to have the default be like it was and the aliases the option instead.
Or could textual have one gov specific logic and display the option differently based on the proposal type?

facundomedica · 2023-12-22T11:11:53Z

@julienrbrt yes, having a custom message renderer is another option:

cosmos-sdk/x/tx/signing/textual/handler.go

Lines 186 to 190 in 19e66a9

    
           // DefineMessageRenderer adds a new custom message renderer. 
        
           func (r *SignModeHandler) DefineMessageRenderer(name protoreflect.FullName, vr ValueRenderer) { 
        
           	r.init() 
        
           	r.messages[name] = vr 
        
           }

I think it would be the first time this gets used :D

EDIT: we'd need to include the proposal type in the message, but it's doable

fix textual tests

a74e299

tac0turtle marked this pull request as ready for review December 21, 2023 00:31

tac0turtle requested a review from a team as a code owner December 21, 2023 00:31

tac0turtle assigned facundomedica and julienrbrt Dec 21, 2023

github-actions bot added the C:x/tx label Dec 21, 2023

julienrbrt previously approved these changes Dec 21, 2023

View reviewed changes

alexanderbez approved these changes Dec 21, 2023

View reviewed changes

facundomedica closed this Dec 22, 2023

julienrbrt deleted the marko/textual_tests branch December 22, 2023 12:05

julienrbrt mentioned this pull request Jan 24, 2024

Textual and Voting Options #19228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: fix textual tests #18849

chore: fix textual tests #18849

tac0turtle commented Dec 21, 2023 •

edited

Loading

julienrbrt Dec 21, 2023

tac0turtle Dec 21, 2023 •

edited

Loading

facundomedica Dec 21, 2023

julienrbrt commented Dec 21, 2023

tac0turtle commented Dec 21, 2023 •

edited

Loading

julienrbrt commented Dec 21, 2023

facundomedica commented Dec 22, 2023

julienrbrt commented Dec 22, 2023

facundomedica commented Dec 22, 2023 •

edited

Loading

chore: fix textual tests #18849

chore: fix textual tests #18849

Conversation

tac0turtle commented Dec 21, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

julienrbrt Dec 21, 2023

Choose a reason for hiding this comment

tac0turtle Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

facundomedica Dec 21, 2023

Choose a reason for hiding this comment

julienrbrt commented Dec 21, 2023

tac0turtle commented Dec 21, 2023 • edited Loading

julienrbrt commented Dec 21, 2023

facundomedica commented Dec 22, 2023

julienrbrt commented Dec 22, 2023

facundomedica commented Dec 22, 2023 • edited Loading

tac0turtle commented Dec 21, 2023 •

edited

Loading

tac0turtle Dec 21, 2023 •

edited

Loading

tac0turtle commented Dec 21, 2023 •

edited

Loading

facundomedica commented Dec 22, 2023 •

edited

Loading