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

[COIN 584] [XTZ] Improve Fees and GasLimit using a RPC node #643

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

Conversation

gagbo
Copy link
Contributor

@gagbo gagbo commented Aug 20, 2020

Fix

Compute the "mean fees" of last block by actually dividing the total fees by the number of non trivial ops

Improvements to gasLimit and fee estimations

  • HttpClient : Send error payloads as well as the http code from Http exceptions (because it make debugging easier down the road)
  • TezosLikeTransactionApi : Add a JSON serializer specific for the run_operation endpoint on a RPC node. The binary serialization did not work well with the endpoint and the error messages are harder to act on
  • TezosLikeExplorers : add interface (implemented only on TZSTATS type of explorer) to fetch the current gasPrice
  • TezosLikeExplorers : add interface (implemented only on TZSTATS type of explorer for now but can be improved) to fetch the gasLimit for a transaction through Node simulation. The code only works for tx that use Reveal and/or Transaction type operations for now
  • TezosLikeAccount : give an interface to fetch the current gasPrice on the network (will throw on non tzstats explorers), and allow the transaction builder to dynamically compute the fees for a transaction.

To make the automatic estimation this is currently an admittedly bad hack to keep the current request API backwards compatible:

  • the signal is to put the GasLimit to "0" in the request
  • the fee price you want to pay should be in the Fees of the request, in picoTez (10^-12 XTZ) per gasUnit ; if you also leave "0" there, then the transaction builder will fetch the gasPrice from the next block (which can be 0 for an empty block)

Then on transaction building, Core detects the 2 info to actually make a transaction with the correct gasLimit and the associated fees according to your fee price. Note that there are no guarantees that the fees amount won't be too high for the sender's balance, callers should check this before signing/broadcasting.

@gagbo gagbo force-pushed the coin-584/xtz-averageFeesLastBlock branch 2 times, most recently from af783b4 to 7787ace Compare August 21, 2020 11:42
core/src/wallet/tezos/TezosLikeAccount2.cpp Outdated Show resolved Hide resolved
core/src/wallet/tezos/TezosLikeAccount2.cpp Outdated Show resolved Hide resolved
core/src/wallet/tezos/TezosLikeAccount2.cpp Outdated Show resolved Hide resolved
core/src/wallet/tezos/TezosLikeAccount2.cpp Outdated Show resolved Hide resolved
core/src/wallet/tezos/TezosLikeAccount2.cpp Outdated Show resolved Hide resolved
Comment on lines +364 to +371
static const auto bogusSignature =
"edsigtkpiSSschcaCt9pUVrpNPf7TTcgvgDEDD6NCEHMy8NNQJCGnMfLZzYoQj74yLjo9wx6MPVV29"
"CvVzgi7qEcEUok3k7AuMg";
vString.SetString(
bogusSignature,
static_cast<SizeType>(std::strlen(bogusSignature)),
allocator);
opObject.AddMember("signature", vString, allocator);
Copy link

Choose a reason for hiding this comment

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

Why do we need to put a fake signature here / what happens if we don't? (Tezos noob question)

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 node refuses to make the operation simulation with an error

core/src/wallet/tezos/api_impl/TezosLikeTransactionApi.cpp Outdated Show resolved Hide resolved
Comment on lines +137 to +133
const std::string picoTezGasPrice = api::BigInt::fromDecimalString(apiGasPrice, 6, ".")->toString(10);
return std::make_shared<BigInt>(std::stoi(picoTezGasPrice));
Copy link

Choose a reason for hiding this comment

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

Why not just return the result of api::BigInt::fromDecimalString here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because when I use BigInt->toInt64 later I don't know if I get 500 or 50 for a value like "0.50"

@ghost
Copy link

ghost commented Sep 1, 2020

(Only the comment on TezosLikeTransactionApi.cpp needs double checking IMO, the rest is probably unimportant).

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Seems all good, for a non-Tezos-expert though.
Thanks for the answers to my questions.

@gagbo gagbo force-pushed the coin-584/xtz-averageFeesLastBlock branch from 89159a4 to 98982fb Compare September 3, 2020 11:07
@gagbo
Copy link
Contributor Author

gagbo commented Sep 3, 2020

HODL it blocks reveal for now

@gagbo gagbo force-pushed the coin-584/xtz-averageFeesLastBlock branch from 98982fb to 7c31a70 Compare October 5, 2020 13:46
@gagbo
Copy link
Contributor Author

gagbo commented Oct 5, 2020

HODL it blocks reveal for now

All issues are fixed and accounted for, but for the time being it's a pile of hacks in TezosLikeExtendedPublicKey::toBase58 and this needs to be fixed properly and defined in unit tests

@gagbo
Copy link
Contributor Author

gagbo commented Sep 3, 2021

TODO: investigate on which branches this code is live

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.

1 participant