Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(statemachine)!: re-implement legacy msg interface #3907

Merged
merged 6 commits into from
Jun 22, 2023

Conversation

crodriguezvega
Copy link
Contributor

@crodriguezvega crodriguezvega commented Jun 21, 2023

Description

Removing the Type() function implementation in this PR broke transaction signing with ledger. This PR adds it back together with some extra type checking to make sure types implement the necessary interfaces. I will open a follow up issue to remove the implementation of LegacyMsg interface when we upgrade to SDK v0.50.x.

closes: #XXXX

Commit Message / Changelog Entry

fix(statemachine)!: re-implement legacy msg interface

see the guidelines for commit messages. (view raw markdown for examples)


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md).
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/).
  • Added relevant godoc comments.
  • Provide a commit message to be used for the changelog entry in the PR description for review.
  • Re-reviewed Files changed in the Github PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

@crodriguezvega crodriguezvega added the type: bug Something isn't working as expected label Jun 21, 2023
func (msg MsgPayPacketFee) Route() string {
return RouterKey
}

// GetSignBytes implements sdk.Msg.
// GetSignBytes implements legacytx.LegacyMsg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring can remain sdk.Msg I believe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it because it looks like GetSignBytes is not part of the sdk.Msg interface, but of the LegacyMsg interface instead...

Copy link
Member

@damiannolan damiannolan Jun 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

either works for me, it will be removed post eden I guess

@asalzmann
Copy link

I will open a follow up issue to remove the implementation of LegacyMsg interface when we upgrade to SDK v0.50.x.

Is this going to require a change to the ledger software, so that it can sign protos?

@damiannolan
Copy link
Member

I will open a follow up issue to remove the implementation of LegacyMsg interface when we upgrade to SDK v0.50.x.

Is this going to require a change to the ledger software, so that it can sign protos?

I'm unsure on this, it may be supported by SIGN_MODE_TEXTUAL but maybe @tac0turtle can provide insight!

@damiannolan damiannolan changed the title fix: re-implement legacy msg interface fix(statemachine)!: re-implement legacy msg interface Jun 21, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2023

Codecov Report

Merging #3907 (75856a2) into main (78be372) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3907      +/-   ##
==========================================
- Coverage   78.88%   78.84%   -0.04%     
==========================================
  Files         187      187              
  Lines       12944    12950       +6     
==========================================
  Hits        10211    10211              
- Misses       2305     2311       +6     
  Partials      428      428              
Impacted Files Coverage Δ
modules/apps/29-fee/types/msgs.go 89.38% <0.00%> (-3.29%) ⬇️
modules/apps/transfer/types/msgs.go 93.22% <0.00%> (-3.28%) ⬇️

@tac0turtle
Copy link
Member

ledger support of sign mode textual is released and audited. We are just waiting for it to be included into ledger live

@crodriguezvega crodriguezvega merged commit f2b2249 into main Jun 22, 2023
@crodriguezvega crodriguezvega deleted the carlos/reimplement-legacy-msg-interface branch June 22, 2023 14:18
mergify bot pushed a commit that referenced this pull request Jun 22, 2023
(cherry picked from commit f2b2249)

# Conflicts:
#	modules/apps/29-fee/types/msgs.go
#	modules/apps/transfer/types/msgs.go
crodriguezvega pushed a commit that referenced this pull request Jun 22, 2023
#3938)

* fix(statemachine)!: re-implement legacy msg interface (#3907)

(cherry picked from commit f2b2249)

# Conflicts:
#	modules/apps/29-fee/types/msgs.go
#	modules/apps/transfer/types/msgs.go

* resolving backport conflicts

---------

Co-authored-by: Carlos Rodriguez <[email protected]>
Co-authored-by: Damian Nolan <[email protected]>
@faddat faddat mentioned this pull request Jun 23, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants