-
Notifications
You must be signed in to change notification settings - Fork 166
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
Transactions signatures that include the transaction domain tag are valid #606
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few suggestions / comments. Looks good!
@@ -1775,3 +1776,35 @@ func TestBlockContext_ExecuteTransaction_FailingTransactions(t *testing.T) { | |||
}), | |||
) | |||
} | |||
func TestSigningWithTags(t *testing.T) { | |||
t.Run("Signing Transactions without tag works (for now)", newVMTest(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test case for signing with a tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually my process was:
- remove ability to sign without a tag
- make sure all tests pass by adding the tag where necessary (I also used a local version of the SDK that added the tag)
- re-add ability to sign without a tag
So all other tests (that do signing) use a tag (or will use it when the SDK version is updated). That is why I only added a test for explicitly not using it.
Do you still think it makes sense to add an explicit test case for using the tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
ref: https://github.com/dapperlabs/flow-go/issues/5204
Description
Right now transaction message is not prepended with a domain tag before signed. For a period of time we want both 'not prepended' and 'prepended' to be valid. This PR makes that change in flow-go
Changes