Jira/coin 1151: Tezos: rework getting balance for originated accounts#679
Jira/coin 1151: Tezos: rework getting balance for originated accounts#679hlafet-ledger wants to merge 3 commits intoLedgerHQ:developfrom
Conversation
hlafet-ledger
changed the title
| } | ||
|
|
||
| void TezosLikeWallet::invalidateBalanceCache(size_t accountIndex, const std::string& originatedAccount) { | ||
| getBalanceCache().erase(fmt::format("{}-{}-{}", getCurrency().name, accountIndex, originatedAccount)); |
There was a problem hiding this comment.
Maybe extract the format in a constant to make sure it's always formatted the same?
There was a problem hiding this comment.
I agree. I will create a single function to format the cache key
| auto value = BigInt(op->getAmount()->toString()); | ||
| auto tzOp = op->asTezosLikeOperation(); | ||
| auto tzTx = tzOp->getTransaction(); | ||
| auto value = (tzTx->getType() == api::TezosOperationTag::OPERATION_TAG_TRANSACTION) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
update_balance is called by a generic code (shared between several coins), we cannot use tezos specific functions (as getType)
There was a problem hiding this comment.
I see. Then maybe quit the function asap?
if (tzTx->getType() != api::TezosOperationTag::OPERATION_TAG_TRANSACTION)
return;
There was a problem hiding this comment.
sum can be impacted by fees (even if value is equal to 0). We cannot quit the function
| if (!localAccount) { | ||
| throw make_exception(api::ErrorCode::NULL_POINTER, "Account was released."); | ||
| } | ||
| auto wallet = std::dynamic_pointer_cast<TezosLikeWallet>(localAccount->getWallet()); |
There was a problem hiding this comment.
we should check for null pointer here to avoid segfaults later
There was a problem hiding this comment.
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.
gagbo
left a comment
gagbo
left a comment
There was a problem hiding this comment.
It looks fine, I just don't understand:
- when the balanceCache is supposed to be invalidated ?
- TTLCache means Time To Live Cache ? Where is the duration for this cache defined ?
| const std::vector<std::string> &matchableKeys); | ||
|
|
||
| virtual Future<std::shared_ptr<BigInt>> | ||
| getBalance(const std::vector<TezosLikeKeychain::Address> &addresses) = 0; |
There was a problem hiding this comment.
Maybe we could keep TezosLikeKeychain::Address instead of std::string to keep some more type-safety?
There was a problem hiding this comment.
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
Yes this is a time to live cache. It is defined in the AbstractWallet class. Duration can be configured through api::Configuration::TTL_CACHE, and default value is 30 seconds |
| @@ -62,9 +63,11 @@ namespace ledger { | |||
| } | |||
|
|
|||
| static inline void update_balance(std::shared_ptr<api::Operation> const& op, BigInt& sum) { | |||
There was a problem hiding this comment.
Maybe rename this function updateBalance for style consistency
There was a problem hiding this comment.
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.
| getBalanceCache().put(fmt::format("{}-{}-{}", getCurrency().name, accountIndex, originatedAccount), balance); | ||
| } | ||
|
|
||
| void TezosLikeWallet::invalidateBalanceCache(size_t accountIndex, const std::string& originatedAccount) { |
There was a problem hiding this comment.
Is this method useful? I don't see any references to it in the code
There was a problem hiding this comment.
Yes it is not called. I added it as a helper function for, may be, a future use.
4 participants
No description provided.