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] [COIN 584] [XTZ] Improve Fees and GasLimit using a RPC node #673

Merged
merged 3 commits into from
Dec 3, 2020

Conversation

twilgenbus-ledger
Copy link
Contributor

@twilgenbus-ledger twilgenbus-ledger commented Nov 30, 2020

This PR provides a way to get much better XTZ gas and fees estimations. It's merely a merge of the work done by @gagbo:
#643

Conflicts:

  • core/src/tezos/TezosLikeExtendedPublicKey.cpp
  • core/src/wallet/tezos/TezosLikeAccount2.cpp
  • core/src/wallet/tezos/api_impl/TezosLikeTransactionApi.cpp
  • core/src/wallet/tezos/explorers/ExternalTezosLikeBlockchainExplorer.cpp

…' into nrt_tezos

# Conflicts:
#	core/src/tezos/TezosLikeExtendedPublicKey.cpp
#	core/src/wallet/tezos/TezosLikeAccount2.cpp
#	core/src/wallet/tezos/api_impl/TezosLikeTransactionApi.cpp
#	core/src/wallet/tezos/explorers/ExternalTezosLikeBlockchainExplorer.cpp
@twilgenbus-ledger twilgenbus-ledger changed the title Merge remote-tracking branch 'Gerry/coin-584/xtz-averageFeesLastBlock… [WIP] [COIN 584] [XTZ] Improve Fees and GasLimit using a RPC node Nov 30, 2020
|| (request.type != api::TezosOperationTag::OPERATION_TAG_DELEGATION && !request.value && !request.wipe)) {
throw make_exception(api::ErrorCode::INVALID_ARGUMENT,
"Missing mandatory informations (e.g. gasLimit, gasPrice or value).");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why removed ?

return self->estimateGasLimit(filledTx).flatMapPtr<api::TezosLikeTransaction>(self->getMainExecutionContext(), [filledTx, gasPrice] (const std::shared_ptr<BigInt> &gas) -> FuturePtr<api::TezosLikeTransaction> {
// 0.000001 comes from the gasPrice->toInt64 being in picoTez
const auto fees = std::make_shared<BigInt>(static_cast<int64_t>(1 + static_cast<double>(gas->toInt64()) * static_cast<double>(gasPrice->toInt64()) * 0.000001));
filledTx->setTransactionGasLimit(gas);
Copy link
Contributor

Choose a reason for hiding this comment

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

we are missing here setRevealGasLimit and setRevealFees

@@ -69,46 +67,73 @@ namespace ledger {
Future<std::shared_ptr<BigInt>>
ExternalTezosLikeBlockchainExplorer::getFees() {
const bool parseNumbersAsString = true;
const auto feesField =
getConfiguration()->getString(api::Configuration::BLOCKCHAIN_EXPLORER_API_ENDPOINT)
Copy link
Contributor

Choose a reason for hiding this comment

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

you are roll-backing here the new dev about the "fee"/"fees" fix

{
// FIXME: await the correct chain id instead of hardcoded value
// ChainID is obtained by doing GET RPCNode /chains/main/chain_id
const auto strChainID = "NetXdQprcVkpaWU";
Copy link
Contributor

Choose a reason for hiding this comment

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

a missed hardcoded value ?

vString.SetString(hash.c_str(), static_cast<SizeType>(hash.length()), allocator);
opObject.AddMember("branch", vString, allocator);

// Fake sign
Copy link
Contributor

Choose a reason for hiding this comment

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

hardcoded value ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fake sign is hardcoded because the signature must be in a format that tezos node can parse, even if it’s never verified

@hlafet-ledger hlafet-ledger merged commit 05c28df into nrt_tezos Dec 3, 2020
@hlafet-ledger hlafet-ledger deleted the nrt_tezos-dyn_gas_fees branch December 3, 2020 13:42
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.

3 participants