search

stacks-archive / stacks-transactions-js

The JavaScript library for generating Stacks 2.0 transactions
19 stars 17 forks source link

refactor: improve error message during fee estimation #79

Closed kyranjamie closed 4 years ago

kyranjamie commented 4 years ago

Not sure if this is intended behaviour or not, but came across a relatively unhelpful error message, while creating a ContractDeploy following the README.

The readme shows no fee in the config object, relying on the default. This causes estimateTransfer to be invoked:

https://github.com/blockstack/stacks-transactions-js/blob/6125026d256d44da0aff966ef82189ca30c1e7b1/src/builders.ts#L85-L87

Does fee estimation only work for TokenTransfers? This same payload check is present on estimateContractDeploy as well.

https://github.com/blockstack/stacks-transactions-js/blob/6125026d256d44da0aff966ef82189ca30c1e7b1/src/builders.ts#L278-L293

Should this check be for PayloadType.ContractDeploy instead?

codecov[bot] commented 4 years ago

Codecov Report

Merging #79 into master will not change coverage. The diff coverage is 16.66%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #79   +/-   ##
=======================================
  Coverage   84.86%   84.86%           
=======================================
  Files          26       26           
  Lines        1434     1434           
  Branches      262      262           
=======================================
  Hits         1217     1217           
  Misses        215      215           
  Partials        2        2
Impacted Files Coverage Δ
src/builders.ts 71.50% <16.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6125026...41b91fb. Read the comment docs.

yknl commented 4 years ago

Does fee estimation only work for TokenTransfers? This same payload check is present on estimateContractDeploy as well.

You're right, it should check for contract deploy and contract function call transaction types. Currently they're all being estimated via the /v2/transfer/fees endpoint because contract transaction fee estimation is not implemented yet.

kyranjamie commented 4 years ago

Cool, thanks for explanation. Updated for all of them.