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

[WIP]Add NULS address and transaction signature C++ impl #193

Closed
wants to merge 79 commits into from
Closed

[WIP]Add NULS address and transaction signature C++ impl #193

wants to merge 79 commits into from

Conversation

daviyang35
Copy link
Contributor

Description

First PR.

Add NULS address and transaction signature C++ impl

Testing instructions

Types of changes

New feature (non-breaking change which adds functionality)

Add NULS Address and Signer.
Add NULS Address/Signer unittest.
Add NULS Address/Signer C Interface.

Checklist

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.

encode64LE(val & 0xffffffffffff, data) will write 8 byte data to buffer.
So we use encode48LE to write 6 byte number.

src/proto/NULS.proto Outdated Show resolved Hide resolved
src/proto/NULS.proto Outdated Show resolved Hide resolved
@daviyang35
Copy link
Contributor Author

already fix spell and field data type error

Copy link
Contributor

@alejandro-isaza alejandro-isaza left a comment

Choose a reason for hiding this comment

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

Please fill in the checklist:

  • Implement functionality in C++. Put it in a subfolder of src/.
    • Address (if necessary)
    • Transaction (if necessary)
    • Signer
  • Write unit tests. Put them in a subfolder of tests/.
    [ ] Mnemonic phrase - > Address derivation test. Put this test in the CoinTests.cpp file
  • Add relevant constants in TWCoinType, TWP2SHPrefix, TWEthereymChainID, TWHRP, etc., as necessary.
  • Return correct curve and purpose in src/Coin.cpp.
  • Implement address validation and derivation in src/Coin.cpp.
  • Implement coin configuration src/include/TWCoinTypeConfiguration.cpp.
  • Write interface header in include/TrustWalletCore and implement the interface in src/interface.
    • Address interface (if necessary).
    • Signing interface.
  • Validate generated code in Android an iOS projects. Write integration tests for each.

src/NULS/Address.cpp Outdated Show resolved Hide resolved
src/NULS/Address.cpp Outdated Show resolved Hide resolved
src/NULS/Address.cpp Outdated Show resolved Hide resolved
src/NULS/Address.cpp Outdated Show resolved Hide resolved
src/NULS/Signer.cpp Outdated Show resolved Hide resolved
alejandro-isaza and others added 5 commits March 21, 2019 10:18
Co-Authored-By: daviyang35 <daviyang35@gmail.com>
Co-Authored-By: daviyang35 <daviyang35@gmail.com>
Co-Authored-By: daviyang35 <daviyang35@gmail.com>
Co-Authored-By: daviyang35 <daviyang35@gmail.com>
@daviyang35
Copy link
Contributor Author

  • Implement functionality in C++. Put it in a subfolder of src/.
    • Address (if necessary)
    • Transaction (if necessary)
    • Signer
  • Write unit tests. Put them in a subfolder of tests/.
    [x] Mnemonic phrase - > Address derivation test. Put this test in the CoinTests.cpp file
  • Add relevant constants in TWCoinType, TWP2SHPrefix, TWEthereymChainID, TWHRP, etc., as necessary.
  • Return correct curve and purpose in src/Coin.cpp.
  • Implement address validation and derivation in src/Coin.cpp.
  • Implement coin configuration src/include/TWCoinTypeConfiguration.cpp.
  • Write interface header in include/TrustWalletCore and implement the interface in src/interface.
    • Address interface (if necessary).
    • Signing interface.
  • Validate generated code in Android an iOS projects. Write integration tests for each.

src/NULS/Address.cpp Outdated Show resolved Hide resolved

#include <sys/time.h>

int64_t getCurrentTime()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this, and add a timestamp field to TransactionInput

package TW.NULS.Proto;
option java_package = "wallet.core.jni.proto";

message TransactionInput {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please change this to SigningInput? like other chains

else if (tx.outputs_size()==0) {
throw std::invalid_argument("There must be at least one output, something is missed");
}
auto priv = Address::importHexPrivateKey(tx.private_key());
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need importHexPrivateKey, you can add a private_key to SigningInput and get it from signer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are 3 possibilities for the private key exported by nuls wallet. If the import is used directly, it will fail because the length does not meet the requirements.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's just consider the simplest case that we derive a private key from mnemonic, @vikmeup what do you think?

* @param remarkSize UTF-8 encode remark string length. Empty is 0
* @return needed fee
*/
static uint64_t getFee(uint32_t inputCount, uint32_t outputCount, uint32_t remarkSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please check and use TransactionPlan and UnspentCalculator? We had same problems when calculating fees for bitcoin and zcash

@daviyang35
Copy link
Contributor Author

@hewigovens I add TransactionBuilder help you make transaction.

# Conflicts:
#	JNI/cpp/HDWallet.c
#	JNI/cpp/HDWallet.h
#	JNI/cpp/PublicKey.c
#	JNI/cpp/PublicKey.h
#	JNI/cpp/StellarAddress.c
#	JNI/cpp/StellarAddress.h
#	JNI/java/wallet/core/jni/CoinType.java
#	JNI/java/wallet/core/jni/HDWallet.java
#	JNI/java/wallet/core/jni/PublicKey.java
#	JNI/java/wallet/core/jni/StellarAddress.java
#	JNI/java/wallet/core/jni/proto/Binance.java
#	TrustWalletCore.podspec
#	android/gradle.properties
#	include/TrustWalletCore/TWHDWallet.h
#	src/Base58.h
#	src/Bitcoin/Address.cpp
#	src/Bitcoin/Address.h
#	src/Bitcoin/Script.cpp
#	src/Coin.cpp
#	src/Coin.h
#	src/Data.h
#	src/Decred/Address.cpp
#	src/Ethereum/Address.cpp
#	src/HDWallet.cpp
#	src/HDWallet.h
#	src/Hash.h
#	src/Keystore/Account.cpp
#	src/NEO/Address.h
#	src/Tezos/Address.cpp
#	src/Tezos/Address.h
#	src/Tezos/Forging.cpp
#	src/Theta/Transaction.h
#	src/Tron/Address.h
#	src/Zcash/TAddress.cpp
#	src/Zcash/TAddress.h
#	src/interface/TWCoinTypeConfiguration.cpp
#	src/interface/TWPrivateKey.cpp
#	src/interface/TWStellarAddress.cpp
#	swift/Sources/Addresses/WanchainAddressExtension.swift
#	swift/Sources/Binance.pb.swift
#	swift/Sources/Blockchains/Tron.swift
#	swift/Sources/CoinType.swift
#	swift/Sources/HDWallet.swift
#	swift/Sources/PublicKey.swift
#	swift/Sources/StellarAddress.swift
#	swift/Tests/CoinAddressDerivationTests.swift
#	tests/CoinTests.cpp
#	tests/interface/TWCoinTypeConfigTests.cpp
#	tests/interface/TWHDWalletTests.cpp
#	tests/interface/TWRippleTests.cpp
#	tests/interface/TWZcashTests.cpp
#	tools/lint
@vikmeup
Copy link
Contributor

vikmeup commented Apr 5, 2019

@daviyang35 can you rebase against master and remove generated files? we don't commit those anymore

@daviyang35
Copy link
Contributor Author

@vikmeup I'm clean some generate file already.

@codecov
Copy link

codecov bot commented Apr 7, 2019

Codecov Report

Merging #193 into master will increase coverage by 0.42%.
The diff coverage is 89.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #193      +/-   ##
==========================================
+ Coverage    79.3%   79.72%   +0.42%     
==========================================
  Files         184      188       +4     
  Lines        4435     4621     +186     
==========================================
+ Hits         3517     3684     +167     
- Misses        918      937      +19
Impacted Files Coverage Δ
src/interface/TWCoinTypeConfiguration.cpp 77.22% <0%> (-1.76%) ⬇️
src/NULS/Signer.h 100% <100%> (ø)
src/Coin.cpp 73% <100%> (+0.55%) ⬆️
src/NULS/Address.cpp 84.9% <84.9%> (ø)
src/NULS/TransactionBuilder.h 92.72% <92.72%> (ø)
src/NULS/Signer.cpp 94.28% <94.28%> (ø)
src/Base58.h 100% <0%> (ø) ⬆️
... and 2 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 b1b876b...1fff601. Read the comment docs.

@hewigovens
Copy link
Contributor

@daviyang35 hi, could you please fix the Android CI?

@daviyang35
Copy link
Contributor Author

@hewigovens Process done.

Copy link
Contributor

@hewigovens hewigovens left a comment

Choose a reason for hiding this comment

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

@daviyang35 I strongly recommend you to branch out from latest master and submit a new pr you don't know how to rebase, too many unrelated changes:

include/TrustWalletCore/DecredSigner.c
include/TrustWalletCore/DecredSigner.h
include/TrustWalletCore/OntologySigner.c
...
swift/Sources/CoinType.swift
...

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.