Skip to content
This repository was archived by the owner on Nov 5, 2024. It is now read-only.

Conversation

@jribbink
Copy link
Contributor

@jribbink jribbink commented Jul 25, 2022

Closes #154

Description

  • Allows signers to be provided as either an address (like before) or an object including any of the following properties: addr, privateKey, hashAlgorithm, keyId
  • Fixes dev-tests (were not being used by jest config previously)
  • Adds new dev-tests for signers provided as object
  • Adds documentation to sendTransaction

For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@changeset-bot
Copy link

changeset-bot bot commented Jul 25, 2022

🦋 Changeset detected

Latest commit: e3d32f7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@onflow/flow-js-testing Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jribbink jribbink force-pushed the jribbink/signers-upgrades branch from 790f3e4 to 5193e69 Compare July 25, 2022 23:44
@jribbink jribbink changed the title Jribbink/signers upgrades Allow for more complex transaction signers to be provided Jul 25, 2022
@jribbink jribbink changed the title Allow for more complex transaction signers to be provided Allow for custom transaction signers to be provided Jul 25, 2022
@jribbink jribbink force-pushed the jribbink/signers-upgrades branch from 3d8e24c to e3d32f7 Compare July 25, 2022 23:56
@jribbink jribbink added the enhancement New feature or request label Jul 25, 2022
@jribbink jribbink marked this pull request as ready for review July 25, 2022 23:57
@jribbink jribbink requested a review from a team as a code owner July 25, 2022 23:57
@jribbink jribbink requested a review from gregsantos July 26, 2022 16:59
Copy link

@gregsantos gregsantos left a comment

Choose a reason for hiding this comment

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

It shall be approved

@jribbink jribbink merged commit 9dcab53 into master Jul 26, 2022
@jribbink jribbink deleted the jribbink/signers-upgrades branch July 26, 2022 20:37
@tav
Copy link

tav commented Jul 27, 2022

Thanks for the super fast turnaround!

Unfortunately, there seems to be some confusion around the cryptographic terminology:

  • hashAlgorithm refers to one of the supported hash algorithms, i.e. SHA2_256, SHA2_384, SHA3_256, SHA3_384, KMAC128_BLS_BLS12_381, and KECCAK_256.

  • keyType or signingAlgorithm refers to one of the supported signing algorithms, i.e. ECDSA_P256, ECDSA_secp256k1, and BLS_BLS12_381.

That is, what this PR has called hashAlgorithm, should actually be something like keyType or signingAlgorithm, and there should be a separate hashAlgorithm field which results in the right hashing function being used.

@jribbink
Copy link
Contributor Author

Thanks for the super fast turnaround!

Unfortunately, there seems to be some confusion around the cryptographic terminology:

  • hashAlgorithm refers to one of the supported hash algorithms, i.e. SHA2_256, SHA2_384, SHA3_256, SHA3_384, KMAC128_BLS_BLS12_381, and KECCAK_256.
  • keyType or signingAlgorithm refers to one of the supported signing algorithms, i.e. ECDSA_P256, ECDSA_secp256k1, and BLS_BLS12_381.

That is, what this PR has called hashAlgorithm, should actually be something like keyType or signingAlgorithm, and there should be a separate hashAlgorithm field which results in the right hashing function being used.

Apologies, this was my mistake and I will try to make a revised PR ASAP. Thank you for the clarification.

@tav
Copy link

tav commented Jul 27, 2022

Thanks @jribbink — any chance you could also do an npm publish too please?

@jribbink
Copy link
Contributor Author

jribbink commented Jul 27, 2022

Thanks @jribbink — any chance you could also do an npm publish too please?

Will do - the package has been moved from https://www.npmjs.com/package/flow-js-testing to https://www.npmjs.com/package/@onflow/flow-js-testing, so the published package will appear in this new registry.

@tav
Copy link

tav commented Jul 28, 2022

Ah, didn't catch the move — thanks @jribbink!

@github-actions github-actions bot mentioned this pull request Aug 3, 2022
This was referenced Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for custom signers for sendTransaction

4 participants