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

Jump to bottom

Jira/coin 1151: Tezos: rework getting balance for originated accounts #679

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Jira/coin 1151: Tezos: rework getting balance for originated accounts #679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
378b53f
COIN-852: Tezos: bad behavior (balance gets to zero) when the server …
hlafet-ledger Dec 1, 2020
56a3fb4
COIN-1151: rework getting balance for originated accounts
hlafet-ledger Dec 17, 2020
0ba7441
add a function to calculate the balance cache key
hlafet-ledger Jan 4, 2021
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
5 changes: 4 additions & 1 deletion core/src/wallet/common/AbstractWallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,6 @@ namespace ledger {
return asInstanceOf<StellarLikeWallet>();
}


Option<Amount> AbstractWallet::getBalanceFromCache(size_t accountIndex) {
return _balanceCache.get(fmt::format("{}-{}", _currency.name, accountIndex));
}
Expand All @@ -392,5 +391,9 @@ namespace ledger {
return getConfig();
}

TTLCache<std::string, Amount>& AbstractWallet::getBalanceCache() {
return _balanceCache;
}

}
}
1 change: 1 addition & 0 deletions core/src/wallet/common/AbstractWallet.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ namespace ledger {
protected:
virtual std::shared_ptr<AbstractAccount> createAccountInstance(soci::session& sql, const std::string& accountUid) = 0;
void addAccountInstanceToInstanceCache(const std::shared_ptr<AbstractAccount>& account);
TTLCache<std::string, Amount>& getBalanceCache();

private:
std::string _name;
Expand Down
3 changes: 1 addition & 2 deletions core/src/wallet/tezos/TezosLikeAccount.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,10 +218,9 @@ namespace ledger {
if (cachedBalance.hasValue()) {
return FuturePtr<Amount>::successful(std::make_shared<Amount>(cachedBalance.getValue()));
}
std::vector<TezosLikeKeychain::Address> listAddresses{_keychain->getAddress()};
auto currency = getWallet()->getCurrency();
auto self = getSelf();
return _explorer->getBalance(listAddresses).mapPtr<Amount>(getMainExecutionContext(), [self, currency](
return _explorer->getBalance(_keychain->getAddress()->toBase58()).mapPtr<Amount>(getMainExecutionContext(), [self, currency](
const std::shared_ptr<BigInt> &balance) -> std::shared_ptr<Amount> {
Amount b(currency, 0, BigInt(balance->toString()));
self->getWallet()->updateBalanceCache(self->getIndex(), b);
Expand Down
2 changes: 1 addition & 1 deletion core/src/wallet/tezos/TezosLikeAccount2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ namespace ledger {
// Check if balance is sufficient
auto currency = self->getWallet()->getCurrency();
auto accountAddress = TezosLikeAddress::fromBase58(senderAddress, currency);
return explorer->getBalance(std::vector<std::shared_ptr<TezosLikeAddress>>{accountAddress}).flatMapPtr<api::TezosLikeTransaction>(
return explorer->getBalance(senderAddress).flatMapPtr<api::TezosLikeTransaction>(
self->getMainExecutionContext(),
[self, request, explorer, accountAddress, currency, senderAddress](const std::shared_ptr<BigInt> &balance) {
// Check if all needed values are set
Expand Down
16 changes: 16 additions & 0 deletions core/src/wallet/tezos/TezosLikeWallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,5 +159,21 @@ namespace ledger {
return _explorer;
}

const std::string TezosLikeWallet::getCacheKey(size_t accountIndex, const std::string& originatedAccount) {
return fmt::format("{}-{}-{}", getCurrency().name, accountIndex, originatedAccount);
}

Option<Amount> TezosLikeWallet::getBalanceFromCache(size_t accountIndex, const std::string& originatedAccount) {
return getBalanceCache().get(getCacheKey(accountIndex, originatedAccount));
}

void TezosLikeWallet::updateBalanceCache(size_t accountIndex, const std::string& originatedAccount, Amount balance) {
getBalanceCache().put(getCacheKey(accountIndex, originatedAccount), balance);
}

void TezosLikeWallet::invalidateBalanceCache(size_t accountIndex, const std::string& originatedAccount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this method useful? I don't see any references to it in the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is not called. I added it as a helper function for, may be, a future use.

getBalanceCache().erase(getCacheKey(accountIndex, originatedAccount));
}

}
}
8 changes: 8 additions & 0 deletions core/src/wallet/tezos/TezosLikeWallet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,14 @@ namespace ledger {

std::shared_ptr<TezosLikeBlockchainExplorer> getBlockchainExplorer();

const std::string getCacheKey(size_t accountIndex, const std::string& originatedAccount);

Option<Amount> getBalanceFromCache(size_t accountIndex, const std::string& originatedAccount);

void updateBalanceCache(size_t accountIndex, const std::string& originatedAccount, Amount balance);

void invalidateBalanceCache(size_t accountIndex, const std::string& originatedAccount);

protected:
std::shared_ptr<AbstractAccount>
createAccountInstance(soci::session &sql, const std::string &accountUid) override;
Expand Down
30 changes: 19 additions & 11 deletions core/src/wallet/tezos/delegation/TezosLikeOriginatedAccount.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#include <api/TezosLikeOperation.hpp>
#include <api/TezosLikeTransaction.hpp>
#include <wallet/pool/WalletPool.hpp>
#include <wallet/tezos/TezosLikeWallet.h>

namespace ledger {
namespace core {
Expand All @@ -62,9 +63,11 @@ namespace ledger {
}

static inline void update_balance(std::shared_ptr<api::Operation> const& op, BigInt& sum) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename this function updateBalance for style consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is called through a template call (see getBalanceHistoryFor in core/src/wallet/common/BalanceHistory.h)
If I change the name here, i have to modify it everywhere including in ethereum code.

auto value = BigInt(op->getAmount()->toString());
auto tzOp = op->asTezosLikeOperation();
auto tzTx = tzOp->getTransaction();
auto value = (tzTx->getType() == api::TezosOperationTag::OPERATION_TAG_TRANSACTION)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could just not call update_balance when that was not a OPERATION_TAG_TRANSACTION?
Since we know this will not change the sum anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update_balance is called by a generic code (shared between several coins), we cannot use tezos specific functions (as getType)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Then maybe quit the function asap?

if (tzTx->getType() != api::TezosOperationTag::OPERATION_TAG_TRANSACTION)
    return;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sum can be impacted by fees (even if value is equal to 0). We cannot quit the function

? BigInt(op->getAmount()->toString())
: BigInt::ZERO;

switch (op->getOperationType()) {
case api::OperationType::RECEIVE: {
Expand Down Expand Up @@ -142,19 +145,24 @@ namespace ledger {
}

FuturePtr<api::Amount> TezosLikeOriginatedAccount::getBalance(const std::shared_ptr<api::ExecutionContext>& context) {
return std::dynamic_pointer_cast<OperationQuery>(queryOperations()->complete())->execute()
.mapPtr<api::Amount>(context, [] (const std::vector<std::shared_ptr<api::Operation>> &ops) {
auto result = BigInt::ZERO;

for (auto const &op : ops) {
OperationStrategy::update_balance(op, result);
}

return std::make_shared<Amount>(currencies::TEZOS, 0, result);
auto localAccount = _originatorAccount.lock();
if (!localAccount) {
throw make_exception(api::ErrorCode::NULL_POINTER, "Account was released.");
}
auto wallet = std::dynamic_pointer_cast<TezosLikeWallet>(localAccount->getWallet());
Copy link
Contributor

Choose a reason for hiding this comment

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

we should check for null pointer here to avoid segfaults later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only case that I can see to get a sigfault here is to have a TezosLikeAccount inside a wallet different from a TezosLikeWallet, which is not possible.
I know that we should check for null after a dynamic cast when we are not sure about type matching. But here we are sure about the pointer type.

auto cachedBalance = wallet->getBalanceFromCache(localAccount->getIndex(), _address);
if (cachedBalance.hasValue()) {
return FuturePtr<api::Amount>::successful(std::make_shared<Amount>(cachedBalance.getValue()));
}
const auto address = _address;
return wallet->getBlockchainExplorer()->getBalance(_address).mapPtr<api::Amount>(context, [wallet, localAccount, address](
const std::shared_ptr<BigInt> &balance) -> std::shared_ptr<Amount> {
Amount b(wallet->getCurrency(), 0, BigInt(balance->toString()));
wallet->updateBalanceCache(localAccount->getIndex(), address, b);
return std::make_shared<Amount>(b);
});
}


void TezosLikeOriginatedAccount::getBalanceHistory(const std::chrono::system_clock::time_point & start,
const std::chrono::system_clock::time_point & end,
api::TimePeriod period,
Expand Down
38 changes: 15 additions & 23 deletions core/src/wallet/tezos/explorers/ExternalTezosLikeBlockchainExplorer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,8 @@ namespace ledger {


Future<std::shared_ptr<BigInt>>
ExternalTezosLikeBlockchainExplorer::getBalance(const std::vector<TezosLikeKeychain::Address> &addresses) {
auto size = addresses.size();
if (size != 1) {
throw make_exception(api::ErrorCode::INVALID_ARGUMENT,
"Can only get balance of 1 address from Tezos Node, but got {} addresses",
addresses.size());
}
std::string addressesStr = addresses[0]->toString();
return getHelper(fmt::format("account/{}", addressesStr),
ExternalTezosLikeBlockchainExplorer::getBalance(const std::string &address) {
return getHelper(fmt::format("account/{}", address),
"total_balance",
std::unordered_map<std::string, std::string>{},
"0",
Expand Down Expand Up @@ -224,6 +217,7 @@ namespace ledger {
const std::string &forceUrl,
bool isDecimal) {
const bool parseNumbersAsString = true;
const bool ignoreStatusCode = true;
auto networkId = getNetworkParameters().Identifier;

std::string p, separator = "?";
Expand All @@ -235,9 +229,19 @@ namespace ledger {
return _http->GET(url + p,
std::unordered_map<std::string, std::string>(),
forceUrl)
.json(parseNumbersAsString)
.json(parseNumbersAsString, ignoreStatusCode)
.mapPtr<BigInt>(getContext(),
[field, networkId, fallbackValue, isDecimal](const HttpRequest::JsonResult &result) {
auto& connection = *std::get<0>(result);
if (connection.getStatusCode() == 404) {
// it means that it’s a “logical” error (i.e. some resources not found), which
// in this case we fallback to a given value
return std::make_shared<BigInt>(!fallbackValue.empty() ? fallbackValue : "0");
}
else if (connection.getStatusCode() < 200 || connection.getStatusCode() >= 300) {
throw Exception(api::ErrorCode::HTTP_ERROR, connection.getStatusText());
}

auto &json = *std::get<1>(result);
if ((!json.IsObject() ||
!json.HasMember(field.c_str()) ||
Expand All @@ -253,19 +257,7 @@ namespace ledger {
value = api::BigInt::fromDecimalString(value, 6, ".")->toString(10);
}
return std::make_shared<BigInt>(value);
})
.recover(getContext(), [fallbackValue] (const Exception &exception) {
auto ecode = exception.getErrorCode();
if (ecode == api::ErrorCode::UNABLE_TO_CONNECT_TO_HOST) {
// if it’s an HTTP error, it might be due to the host not being reachable or such,
// so we re-run the error
throw exception;
}

// otherwise, it means that it’s a “logical” error (i.e. some resources not found), which
// in this case we fallback to a given value
return std::make_shared<BigInt>(!fallbackValue.empty() ? fallbackValue : "0");
});
});
}

Future<std::shared_ptr<BigInt>>
Expand Down
2 changes: 1 addition & 1 deletion core/src/wallet/tezos/explorers/ExternalTezosLikeBlockchainExplorer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ namespace ledger {
const std::shared_ptr<api::DynamicObject> &configuration);

Future<std::shared_ptr<BigInt>>
getBalance(const std::vector<TezosLikeKeychain::Address> &addresses) override;
getBalance(const std::string &address) override;

Future<std::shared_ptr<BigInt>>
getFees() override;
Expand Down
14 changes: 4 additions & 10 deletions core/src/wallet/tezos/explorers/NodeTezosLikeBlockchainExplorer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,23 +50,17 @@ namespace ledger {


Future<std::shared_ptr<BigInt>>
NodeTezosLikeBlockchainExplorer::getBalance(const std::vector<TezosLikeKeychain::Address> &addresses) {
auto size = addresses.size();
if (size != 1) {
throw make_exception(api::ErrorCode::INVALID_ARGUMENT,
"Can only get balance of 1 address from Tezos Node, but got {} addresses", addresses.size());
}
NodeTezosLikeBlockchainExplorer::getBalance(const std::string& address) {
bool parseNumbersAsString = true;
std::string addressesStr = addresses[0]->toBase58();
return _http->GET(fmt::format("blockchain/{}/{}/balance/{}",
getExplorerVersion(),
getNetworkParameters().Identifier,
addressesStr))
address))
.json(parseNumbersAsString)
.mapPtr<BigInt>(getContext(), [addressesStr](const HttpRequest::JsonResult &result) {
.mapPtr<BigInt>(getContext(), [address](const HttpRequest::JsonResult &result) {
auto &json = *std::get<1>(result);
if (!json.IsArray() && json.Size() == 1 && json[0].IsString()) {
throw make_exception(api::ErrorCode::HTTP_ERROR, "Failed to get balance for {}", addressesStr);
throw make_exception(api::ErrorCode::HTTP_ERROR, "Failed to get balance for {}", address);
}
auto info = json[0].GetString();
return std::make_shared<BigInt>(info);
Expand Down
2 changes: 1 addition & 1 deletion core/src/wallet/tezos/explorers/NodeTezosLikeBlockchainExplorer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ namespace ledger {
const std::shared_ptr<api::DynamicObject> &configuration);

Future<std::shared_ptr<BigInt>>
getBalance(const std::vector<TezosLikeKeychain::Address> &addresses) override;
getBalance(const std::string &address) override;

Future<std::shared_ptr<BigInt>>
getFees() override;
Expand Down
2 changes: 1 addition & 1 deletion core/src/wallet/tezos/explorers/TezosLikeBlockchainExplorer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ namespace ledger {
const std::vector<std::string> &matchableKeys);

virtual Future<std::shared_ptr<BigInt>>
getBalance(const std::vector<TezosLikeKeychain::Address> &addresses) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could keep TezosLikeKeychain::Address instead of std::string to keep some more type-safety?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1- As you can see for all other functions in this class, address was always a string.
2- the old format was using a std::vector which must contais only one element (else an error)
So to keep the code simple and consistent, I changed the signature of this function

getBalance(const std::string &address) = 0;

virtual Future<std::shared_ptr<BigInt>>
getFees() = 0;
Expand Down
4 changes: 2 additions & 2 deletions core/test/integration/synchronization/tezos_synchronization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ TEST_F(TezosLikeWalletSynchronization, MediumXpubSynchronization) {
api::EventCode::SYNCHRONIZATION_SUCCEED);

auto balance = wait(account->getBalance());
EXPECT_NE(balance->toLong(), 0L);
EXPECT_GT(balance->toLong(), 0L);

auto originatedAccounts = account->getOriginatedAccounts();
EXPECT_GE(originatedAccounts.size(), 2);
Expand All @@ -112,7 +112,7 @@ TEST_F(TezosLikeWalletSynchronization, MediumXpubSynchronization) {
std::cout << ">>> Nb of originated ops: " << origOps.size() << std::endl;

auto origBalance = wait(std::dynamic_pointer_cast<TezosLikeOriginatedAccount>(origAccount)->getBalance(dispatcher->getMainExecutionContext()));
EXPECT_NE(origBalance->toLong(), 0L);
EXPECT_GT(origBalance->toLong(), 0L);
std::cout << ">>> Originated Balance: " << origBalance->toString() << std::endl;

auto fromDate = DateUtils::fromJSON("2019-02-01T13:38:23Z");
Expand Down