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.

Sign up for GitHub

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

Bitcoin Feature #355

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Bitcoin Feature #355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion core/src/wallet/bitcoin/keychains/CommonBitcoinLikeKeychains.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,8 @@ namespace ledger {
uint32_t to) {
auto maxObservableIndex = (purpose == KeyPurpose::CHANGE ? _state.maxConsecutiveChangeIndex + _state.nonConsecutiveChangeIndexes.size() : _state.maxConsecutiveReceiveIndex + _state.nonConsecutiveReceiveIndexes.size()) + _observableRange;
auto length = std::min<size_t >(to - from, maxObservableIndex - from);
std::vector<BitcoinLikeKeychain::Address> result(length +1);
std::vector<BitcoinLikeKeychain::Address> result;
hadronized marked this conversation as resolved.
Show resolved Hide resolved
result.reserve(length + 1);
for (auto i = 0; i <= length; i++) {
if (purpose == KeyPurpose::RECEIVE) {
result.push_back(derive(KeyPurpose::RECEIVE, from + i));
Expand Down
17 changes: 15 additions & 2 deletions core/src/wallet/bitcoin/transaction_builders/BitcoinLikeUtxoPicker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,21 @@ namespace ledger {

if (buddy->changeAmount > BigInt(_currency.bitcoinLikeNetworkParameters.value().DustAmount)) {
// TODO implement multi change
// TODO implement use specific change address
auto changeAddress = buddy->keychain->getFreshAddress(BitcoinLikeKeychain::CHANGE)->toString();
// If multiple change paths provided we take the last one
auto changeAddressIndex = Try<uint32_t>::from([&](){
if (buddy->request.changePaths.empty()) {
throw make_exception(api::ErrorCode::INVALID_ARGUMENT, "No change path provided by user.");
}
auto it = buddy->request.changePaths.back();
return std::stoul(it.substr(it.find_last_of('/') + 1), nullptr, 0);
Copy link
Contributor

@alekece alekece Oct 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Which version of c++ we are using here ? Because you can remove the substring construction by using the new std::string_view.
More than that, I think there is a bug. If the last changePaths doesn't have any '/', find_last_of returns string::npos which is the maximum value to size_t so when you add 1 to that value, you got a 0 and your stoul call returns 0.
This is what you want ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It’s a C++17 feature. We can’t use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@phaazon Hence my question to @KhalilBellakrid ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use C++14 so .. SFYL ! :trollface:
Concerning the behaviour, yes this is exactly what I'm looking for, we want support all ways of setting paths, user can input 0/1/8, 1/8 or 8 and we will always retrieve the right index, if someone inputs 8/ or some other invalid path then we would throw an exception on the stoul hence the encapsulation in the Try and in that case we would fall back on previous behaviour (getting a fresh change address)

});
auto changeAddress = changeAddressIndex.isFailure() ?
buddy->keychain->getFreshAddress(BitcoinLikeKeychain::CHANGE)->toString():
buddy->keychain->getAllObservableAddresses(
BitcoinLikeKeychain::CHANGE,
changeAddressIndex.getValue(),
changeAddressIndex.getValue()
)[0]->toString();

auto amount = buddy->changeAmount;
auto script = BitcoinLikeScript::fromAddress(changeAddress, _currency);
Expand Down
32 changes: 30 additions & 2 deletions core/test/integration/transactions/bitcoin_P2PKH_transaction_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,36 @@ TEST_F(BitcoinMakeP2PKHTransaction, Toto) {
builder->sendToAddress(api::Amount::fromLong(currency, 1000), "ms8C1x7qHa3WJM986NKyx267i2LFGaHRZn");
builder->pickInputs(api::BitcoinLikePickingStrategy::DEEP_OUTPUTS_FIRST, 0xFFFFFFFF);
builder->setFeesPerByte(api::Amount::fromLong(currency, 10));
auto f = builder->build();
auto tx = ::wait(f);

// Change as fresh address
auto freshChangeAddress = bla->getKeychain()->getFreshAddress(BitcoinLikeKeychain::CHANGE);
auto changePath = freshChangeAddress->getDerivationPath().value_or("");
EXPECT_GT(changePath.size(), 0);
auto tx = ::wait(builder->build());
auto addressFoundInOutputs = [=] (const std::shared_ptr<api::BitcoinLikeTransaction> &inputTx,
const std::string &inputAddress) {
return std::find_if(inputTx->getOutputs().begin(),
inputTx->getOutputs().end(),
[&] (const std::shared_ptr<api::BitcoinLikeOutput> &out) {
if (!out) {
return false;
}
return out->getAddress().value_or("") == inputAddress;
}
) != inputTx->getOutputs().end();
};
EXPECT_TRUE(addressFoundInOutputs(tx, freshChangeAddress->toString()));

// Change as chosen address
// Compare change address to the one set
auto changeAddress = bla->getKeychain()->getAllObservableAddresses(BitcoinLikeKeychain::CHANGE, 5, 5);
EXPECT_GT(changeAddress.size(), 0);
changePath = changeAddress[0]->getDerivationPath().value_or("");
EXPECT_GT(changePath.size(), 0);
builder->addChangePath(changePath);
tx = ::wait(builder->build());
EXPECT_TRUE(addressFoundInOutputs(tx, changeAddress[0]->toString()));

std::cout << hex::toString(tx->serialize()) << std::endl;
std::cout << tx->getOutputs()[0]->getAddress().value_or("NOP") << std::endl;
auto parsedTx = BitcoinLikeTransactionBuilder::parseRawUnsignedTransaction(wallet->getCurrency(), tx->serialize(), 0);
Expand Down