Skip to content
This repository has been archived by the owner on May 28, 2019. It is now read-only.

Add Bitcoin keys support #13

Merged
merged 3 commits into from
Jul 10, 2018
Merged

Add Bitcoin keys support #13

merged 3 commits into from
Jul 10, 2018

Conversation

alejandro-isaza
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Jul 9, 2018

Codecov Report

Merging #13 into master will decrease coverage by 0.07%.
The diff coverage is 87.56%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #13      +/-   ##
=========================================
- Coverage   85.57%   85.5%   -0.08%     
=========================================
  Files          28      38      +10     
  Lines        1241    1552     +311     
=========================================
+ Hits         1062    1327     +265     
- Misses        179     225      +46
Impacted Files Coverage Δ
Sources/Ethereum/Solidity/ABIError.swift 0% <ø> (ø)
Tests/Ethereum/Solidity/ERC20EncoderTests.swift 100% <ø> (ø)
...Ethereum/Solidity/ENS/ReverseResolverEncoder.swift 100% <ø> (ø)
Tests/Ethereum/Solidity/ENS/ENSEncoderTests.swift 100% <ø> (ø)
Tests/Ethereum/Solidity/ABIEncoderTests.swift 98.57% <ø> (ø)
Sources/Ethereum/EthereumCrypto.m 100% <ø> (ø)
Sources/Ethereum/RLP.swift 58.27% <ø> (ø)
Tests/Ethereum/RLPTests.swift 100% <ø> (ø)
Sources/Ethereum/Solidity/ABIType.swift 78.94% <ø> (ø)
Tests/Ethereum/EthereumCryptoTests.swift 100% <ø> (ø)
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ea97c1...61f1643. Read the comment docs.

Copy link
Contributor

@vikmeup vikmeup left a comment

Choose a reason for hiding this comment

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

Looks great!


import Foundation

public struct BitcoinAddress: Address, Hashable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since both BitcoinAddress and EthereumAddress conform to Hashable, should we make Address conform to Hashable then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No because that will make Address a PAT.

Copy link
Contributor

Choose a reason for hiding this comment

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

what is PAT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

public init() {
data = Data(count: Bitcoin.privateKeySize)
repeat {
let result = data.withUnsafeMutableBytes { SecRandomCopyBytes(kSecRandomDefault, Bitcoin.privateKeySize, $0) }
Copy link
Contributor

Choose a reason for hiding this comment

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

does it remove private key from the memory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, will fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

we do this quite often, maybe having a helper function that will do it?

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 did add a helper function to Data: clear()

@alejandro-isaza alejandro-isaza merged commit aacd59d into master Jul 10, 2018
@alejandro-isaza alejandro-isaza deleted the al/bitcoin branch July 10, 2018 04:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants