search

bcoin-org / bcoin

Javascript bitcoin library for node.js and browsers
https://bcoin.io
Other
3.01k stars 811 forks source link

feat: align 'estimatesmartfee' rpc with bitcoind #1157

Closed emjshrx closed 1 year ago

emjshrx commented 1 year ago

Changes the returned json attribute from 'estimatesmartfee' rpc to align with bitcoind docs . Also added a crude test to verify the same.

Fixes https://github.com/bcoin-org/bcoin/issues/1153

theanmolsharma commented 1 year ago

Thanks, good call. I'll review it this week. While I do that if you can, please scrutinize the other RPCs as well. I know this is a tedious and boring task so no pressure.

emjshrx commented 1 year ago

Thanks, good call. I'll review it this week. While I do that if you can, please scrutinize the other RPCs as well. I know this is a tedious and boring task so no pressure.

Thanks @theanmolsharma . Also I noticed the coverage in RPCs are quite less so can make a test suite for them as well.

emjshrx commented 1 year ago

Changed the test to be in the right file and ran a formatter on the file

theanmolsharma commented 1 year ago

Changed the test to be in the right file and ran a formatter on the file

Please revert back the formatter changes.

theanmolsharma commented 1 year ago

Tested ACK

pinheadmz commented 1 year ago

since this is a breaking change, we must also include a release note in the CHANGELOG.md anyone who is using this API regularly for their application will get wrecked if they upgrade and don't comply.

emjshrx commented 1 year ago

since this is a breaking change, we must also include a release note in the CHANGELOG.md anyone who is using this API regularly for their application will get wrecked if they upgrade and don't comply.

Thanks for bringing this to notice @pinheadmz . Have made the changes.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: +0.01 :tada:

Comparison is base (b005869) 69.55% compared to head (350e6b0) 69.57%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1157 +/- ## ========================================== + Coverage 69.55% 69.57% +0.01% ========================================== Files 158 159 +1 Lines 26603 26607 +4 ========================================== + Hits 18505 18511 +6 + Misses 8098 8096 -2 ``` | [Impacted Files](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1157?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org) | Coverage Δ | | |---|---|---| | [lib/node/rpc.js](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1157?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org#diff-bGliL25vZGUvcnBjLmpz) | `41.96% <ø> (+0.57%)` | :arrow_up: | ... and [7 files with indirect coverage changes](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1157/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

pinheadmz commented 1 year ago

merged https://github.com/bcoin-org/bcoin/commit/014a104899dcd9fafca1bcfea88203ed990a8e90 thank you!