Skip to content

Conversation

@kcentrifugal
Copy link
Contributor

This PR fixes the bchsvexplorer client partially, enough to pass current tests.

More time will be needed to fully bring it in line with the current API. But clearly no one is using this client-- is it worth the work?

@kcentrifugal
Copy link
Contributor Author

kcentrifugal commented Jan 16, 2021

unfortunately duplicating work of #78

but I think there are a few things this PR got right-- I'll comment on the other PR

@ghost
Copy link

ghost commented Jan 16, 2021

At first glance looks good to me.

@xloem
Copy link
Contributor

xloem commented Jan 16, 2021

This differs from #78 because it doesn't include https://github.com/AustEcon/bitsv/pull/78/files#diff-94bb32bc0d347563bc1d3f5169d0c048fef96975c9d641dc96c7a0f324aa10b9R26 (the changed endpoint url) and hence connects to a different service. @kcentrifugal so you know, I emailed bchsvexplorer to ask about a query issue and they said that host is no longer maintained and the bsvbook.guarda service replaces it. That was some months ago. Sorry I haven't dealt with the concerns on my PR yet, feel free to favor this one or combine them or whatnot.

@AustEcon
Copy link
Owner

AustEcon commented Jan 17, 2021

Thanks a million @kcentrifugal

Sorry for lagging behind on this... I have been trying to push another BSV project to its first milestone...

Will get onto this as soon as I can.

@kcentrifugal
Copy link
Contributor Author

This differs from #78 because it doesn't include https://github.com/AustEcon/bitsv/pull/78/files#diff-94bb32bc0d347563bc1d3f5169d0c048fef96975c9d641dc96c7a0f324aa10b9R26 (the changed endpoint url) and hence connects to a different service.

No, https://bchsvexplorer.com/ and https://bsvbook.guarda.co/ are the same service on different domains. They serve the same content. Servers can respond to multiple domains. Anyway, I think it's a minor point.

@AustEcon
Copy link
Owner

Posting this here: https://github.com/guardaco/blockbook/blob/guarda-changes/docs/api.md

Because the documentation is not very easy to find with a simple google search...

Copy link
Owner

@AustEcon AustEcon left a comment

Choose a reason for hiding this comment

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

Don't worry about updating this PR. I will sort it and then I'll see about what can be taken from #78

r = requests.get(cls.MAIN_ADDRESS_TX_IDS.format(address), timeout=DEFAULT_TIMEOUT)
r.raise_for_status() # pragma: no cover
return r.json()['transactions']
return r.json()['txids']
Copy link
Owner

Choose a reason for hiding this comment

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

I will change this to:

        txs = r.json().get('txids')  # if there are no txs in history the 'txids' key doesn't exist
        if txs:
            return txs
        return []

Because wierdly their api drops the 'txids' key if there is no history for they key/address...

def test_get_transactions_main_equal(self):
results = [api.get_transactions(MAIN_ADDRESS_USED1) for api in network_api_main.list_of_apis]
assert all_items_common(results[:100])
assert all_items_common(results)
Copy link
Owner

Choose a reason for hiding this comment

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

Agree

@AustEcon AustEcon merged commit 96bac30 into AustEcon:master Jan 23, 2021
@xloem
Copy link
Contributor

xloem commented Jan 23, 2021

maybe something changed since my support email, or i missed something. thanks for addressing this.

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