Add `get_address_stats` and `get_address_txns` endpoints #104

praveenperera · 2024-10-14T01:17:31Z

No description provided.

src/async.rs

src/api.rs

coveralls · 2024-10-15T12:33:00Z

Pull Request Test Coverage Report for Build 11334093933

Details

98 of 100 (98.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.8%) to 88.846%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/async.rs	13	14	92.86%
src/blocking.rs	13	14	92.86%

Totals
Change from base Build 11054999770:	0.8%
Covered Lines:	1139
Relevant Lines:	1282

💛 - Coveralls

praveenperera · 2024-10-15T18:38:47Z

@oleonardolima addressed the comments, naming is hard I went with AddressTxnSummary for the shared struct name

oleonardolima

It's looking good! I think it all can be squashed into a single commit, e.g.: feat(api): add `/address/:address` and `/address/:address/txs` endpoints

src/api.rs

oleonardolima · 2024-10-15T19:59:32Z

src/api.rs

+    pub mempool_stats: AddressTxnSummary,
+}
+
+#[derive(Debug, Copy, Clone, PartialEq, Eq, Deserialize)]


What do we need the Copy one for?

I like to derive Copy whenever possible especially for libraries, because we don't know how the end user is going to use it. It would be nicer for them to be able to Copy and dereference it instead of cloning it.

I don't see much problem, however, we are not deriving it for other APIs, so would rather keep that standard.

Can we merge this in and I can go through the API and add Copy to the other structs that make sense? @oleonardolima ?

I'm fine with it, I'd like to see what others think about it.

src/async.rs

src/lib.rs

praveenperera · 2024-10-15T20:16:44Z

@oleonardolima renamed and squashed

oleonardolima

I suggested two other changes, other than that it looks good! Thanks for extending the Esplora API.

src/async.rs

src/blocking.rs

praveenperera · 2024-10-17T01:25:39Z

my bad, missed those the last time, i've squashed them together

oleonardolima

tACK 9b6bf23

I tested it locally using mempool.space mainnet API and it worked as expected.

It's unrelated to this PR, but CI is failing on the coverage step.

tnull

Basically LGTM, mod some comments/nits.

src/api.rs

src/blocking.rs

tnull · 2024-10-29T08:46:33Z

src/lib.rs

+            )
+            .unwrap();
+
+        let address_blocking = blocking_client.get_address_stats(&address).unwrap();


This is likely flaky as the transaction that we send via the bitcoind RPC will need some time to propagate through the mempool and to get picked up by electrs. We might want to call get_tx or similar in a loop with exponential back-off until we know the txid is available.

Also nit: could renamed these variables, as they are not in fact addresses themselves, but rather address stats.

doesn't the generate_blocks_and_wait handle this?

let _miner = MINER.lock().await; generate_blocks_and_wait(1);

renamed the variables

doesn't the generate_blocks_and_wait handle this?

It does for the block itself, but there is no guarantee the transaction actually made it into the mempool and progated to electrs when we call get_address_stats (prior to generate_blocks_and_wait).

are we doing this on any of the other tests?

No, the other tests don't seem to query Esplora immediately after submitting a new transaction to bitcoind though. electrs only polls for new data in some interval, which can lead to related flakiness in CI in my experience. Here would be an example how to avoid it: https://github.com/lightningdevkit/ldk-node/blob/9e00f357556e5498625515464a89ef7aea14acdf/tests/common/mod.rs#L354-L365

If you don't want to tackle it here, we should probably open an issue for it though, or, granted, wait until we actually see some flakiness.

@tnull I like the idea of waiting to see if we see the flakiness

As the current test state it seems fine, as it's checking for chain_stats summary, which has values only for transactions already on the chain (which should be already covered by generate_blocks_and_wait() fn).

It'd probably be flaky if we're asserting for mempool_stats as well.

oleonardolima

I noticed some typos in the recent commits, also left some suggestions.

All the new commits probably should be squashed under the first one, as they touch the same files and are basically typos and documentation improvements.

src/api.rs

src/lib.rs

praveenperera · 2024-11-03T15:14:25Z

thanks @oleonardolima merged the typo fixes and squashed it all together

oleonardolima

utACK 4d57c58

It looks good! I left another suggestion to make the documentation equivalent on both async and blocking APIs.

Also, the formatting step is failing on CI, a cargo +nightly fmt --all will probably do.

Once that's fixed I'll give another test and properly tACK it 😁.

src/async.rs

praveenperera · 2024-11-06T18:55:13Z

done @oleonardolima

oleonardolima

tACK 3fdd66c

oleonardolima · 2024-11-15T22:40:42Z

@praveenperera You probably need to rebase it, after #110 got merged. @ValuedMammal Could you also take a look at this one, I think it can be merged before #103.

ValuedMammal

ACK 3fdd66c

oleonardolima reviewed Oct 15, 2024

View reviewed changes

src/async.rs Outdated Show resolved Hide resolved

src/api.rs Outdated Show resolved Hide resolved

praveenperera requested a review from oleonardolima October 15, 2024 15:57

oleonardolima reviewed Oct 15, 2024

View reviewed changes

praveenperera force-pushed the get-address-info branch from ca7c213 to 69caadd Compare October 15, 2024 20:15

praveenperera requested a review from oleonardolima October 15, 2024 20:16

oleonardolima reviewed Oct 16, 2024

View reviewed changes

src/async.rs Outdated Show resolved Hide resolved

src/blocking.rs Outdated Show resolved Hide resolved

praveenperera force-pushed the get-address-info branch from 715e7b7 to 75a5f7b Compare October 17, 2024 01:25

praveenperera force-pushed the get-address-info branch from 75a5f7b to 9b6bf23 Compare October 17, 2024 01:26

praveenperera requested a review from oleonardolima October 17, 2024 15:47

oleonardolima approved these changes Oct 17, 2024

View reviewed changes

oleonardolima assigned praveenperera Oct 17, 2024

oleonardolima added the enhancement New feature or request label Oct 17, 2024

oleonardolima requested review from notmandatory and ValuedMammal October 17, 2024 18:09

tnull reviewed Oct 29, 2024

View reviewed changes

oleonardolima reviewed Oct 31, 2024

View reviewed changes

src/api.rs Outdated Show resolved Hide resolved

src/api.rs Outdated Show resolved Hide resolved

src/api.rs Outdated Show resolved Hide resolved

src/api.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

praveenperera force-pushed the get-address-info branch 2 times, most recently from f9aaf60 to 4d57c58 Compare November 3, 2024 15:13

oleonardolima reviewed Nov 6, 2024

View reviewed changes

src/async.rs Outdated Show resolved Hide resolved

feat(api): add /address/:address and /address/:address/txs endpoints

3fdd66c

praveenperera force-pushed the get-address-info branch from 10ad0a6 to 3fdd66c Compare November 6, 2024 18:54

tnull approved these changes Nov 7, 2024

View reviewed changes

oleonardolima approved these changes Nov 7, 2024

View reviewed changes

ValuedMammal approved these changes Nov 17, 2024

View reviewed changes

ValuedMammal merged commit 9fcaf01 into bitcoindevkit:master Nov 17, 2024
3 of 25 checks passed

praveenperera deleted the get-address-info branch November 17, 2024 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `get_address_stats` and `get_address_txns` endpoints #104

Add `get_address_stats` and `get_address_txns` endpoints #104

praveenperera commented Oct 14, 2024

coveralls commented Oct 15, 2024

praveenperera commented Oct 15, 2024

oleonardolima left a comment

oleonardolima Oct 15, 2024

praveenperera Oct 15, 2024

oleonardolima Oct 17, 2024

praveenperera Oct 17, 2024

oleonardolima Oct 17, 2024

praveenperera commented Oct 15, 2024

oleonardolima left a comment

praveenperera commented Oct 17, 2024

oleonardolima left a comment

tnull left a comment

tnull Oct 29, 2024

praveenperera Oct 29, 2024 •

edited

Loading

praveenperera Oct 29, 2024 •

edited

Loading

tnull Oct 30, 2024

praveenperera Oct 30, 2024

tnull Oct 31, 2024 •

edited

Loading

praveenperera Oct 31, 2024

oleonardolima Oct 31, 2024 •

edited

Loading

oleonardolima left a comment

praveenperera commented Nov 3, 2024

oleonardolima left a comment

praveenperera commented Nov 6, 2024

oleonardolima left a comment

oleonardolima commented Nov 15, 2024

ValuedMammal left a comment

Add get_address_stats and get_address_txns endpoints #104

Add get_address_stats and get_address_txns endpoints #104

Conversation

praveenperera commented Oct 14, 2024

coveralls commented Oct 15, 2024

Pull Request Test Coverage Report for Build 11334093933

Details

💛 - Coveralls

praveenperera commented Oct 15, 2024

oleonardolima left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praveenperera commented Oct 15, 2024

oleonardolima left a comment

Choose a reason for hiding this comment

praveenperera commented Oct 17, 2024

oleonardolima left a comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praveenperera Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

praveenperera Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleonardolima Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

oleonardolima left a comment

Choose a reason for hiding this comment

praveenperera commented Nov 3, 2024

oleonardolima left a comment

Choose a reason for hiding this comment

praveenperera commented Nov 6, 2024

oleonardolima left a comment

Choose a reason for hiding this comment

oleonardolima commented Nov 15, 2024

ValuedMammal left a comment

Choose a reason for hiding this comment

Add `get_address_stats` and `get_address_txns` endpoints #104

Add `get_address_stats` and `get_address_txns` endpoints #104

praveenperera Oct 29, 2024 •

edited

Loading

praveenperera Oct 29, 2024 •

edited

Loading

tnull Oct 31, 2024 •

edited

Loading

oleonardolima Oct 31, 2024 •

edited

Loading