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

replace secp256k1 with bcrypto#secp256k1 #34

Merged
merged 1 commit into from
May 4, 2020
Merged

replace secp256k1 with bcrypto#secp256k1 #34

merged 1 commit into from
May 4, 2020

Conversation

faustbrian
Copy link
Contributor

Possibly addresses #13 as I am currently experiencing this issue which makes it impossible to sign transactions with a server-side application.

@faboweb
Copy link
Contributor

faboweb commented Apr 30, 2020

Does this work in the browser?

@faustbrian
Copy link
Contributor Author

I tried it briefly in chrome and had no issues due to bcrypto also having browser bindings https://github.com/bcoin-org/bcrypto/blob/master/package.json#L95 but would appreciate someone else testing.

Also, could forks be enabled on CircleCI so that the tests could run?

@faustbrian
Copy link
Contributor Author

There 2 more things that could be done to streamline things a bit.

  1. Create a node and browser build to keep things separate.
  2. Replace crypto-js with bcrypto methods to rely only on a single dependency for crypto.

@Bitcoinera
Copy link
Contributor

I tested this with couple different networks on the browser and it is working great.

Awesome job @faustbrian !

@faboweb faboweb merged commit 30ca0e3 into luniehq:develop May 4, 2020
@faboweb
Copy link
Contributor

faboweb commented May 4, 2020

Doing a release later. Good idea about the steps reduction! From a developer experience perspective I would avoid two builds if not necessary. What is your reasoning for having 2?

@faustbrian faustbrian deleted the bcrypto branch May 7, 2020 08:51
@faustbrian
Copy link
Contributor Author

faustbrian commented May 10, 2020

I generally like to keep browser and node.js builds separated to avoid having dependencies or polyfills included that would be useless for node.js or could potentially even cause issues with making wrong assumptions about the environment and its data types.

@faboweb
Copy link
Contributor

faboweb commented May 10, 2020

I published the package

@faboweb
Copy link
Contributor

faboweb commented May 10, 2020

I generally like to keep browser and node.js builds separated to avoid having dependencies or polyfills included that would be useless for node.js or could potentially even cause issues with making wrong assumptions about the environment and its data types.

good argument. happy about a PR doing though. Rn we are a bit busy with other things.

@faustbrian
Copy link
Contributor Author

I generally like to keep browser and node.js builds separated to avoid having dependencies or polyfills included that would be useless for node.js or could potentially even cause issues with making wrong assumptions about the environment and its data types.

good argument. happy about a PR doing though. Rn we are a bit busy with other things.

Will try to find some time for it in the next days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants