-
Notifications
You must be signed in to change notification settings - Fork 96
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
Improve signing process for a fully signed transaction - Closes #4059 #4070
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.
LGTM
|
||
it('should return the tx ready to be broadcasted', () => { | ||
expect(signatureSkipped(props)).toEqual( | ||
expect.objectContaining({ type: actionTypes.signatureSkipped }), |
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.
Would be good to add data: binaryTx
be added to the assertion
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.
I don't perform any logic there and that part is actually extensively tested in utils/transaction.js. I think it's redundant, but if you still believe I have to test it I can add that too.
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.
Ah, one more thing, the use of flattenTransaction
and createTransactionObject
is really getting out of hand. We need a proper refactoring round there. I have to change all these tests anyways. Otherwise I don't know how we can handle the sapphire phase.
fce2a4a
to
9e3c140
Compare
What was the problem?
This PR resolves #4059
How was it solved?
How was it tested?