Skip to content

Conversation

@xloem
Copy link
Contributor

@xloem xloem commented Dec 9, 2020

I was having an issue and emailed bchsvexplorer, and guarda.co's support told me that bchsvexplorer is outdated, and that bsvbook.guarda.co replaces it with a modern api.

@xloem xloem mentioned this pull request Dec 9, 2020
@ghost
Copy link

ghost commented Dec 9, 2020

Not familiar with bsvbookguarda, but looks good at first glance.

@AustEcon
Copy link
Owner

Great. Thanks for keeping on top of this.

I'm stretched quite thin just at the moment. Can I come back to this on the weekend?

@xloem
Copy link
Contributor Author

xloem commented Dec 10, 2020

It's the new version of bchsvexplorer according to the support team at guarda.co, who run it. Regarding the weekend, of course, thanks for touching base.

'https://bchsvexplorer.com/api/tx/send',
data=json.dumps({'rawtx': rawtx}),
headers=cls.headers,
MAIN_TX_PUSH_API,
Copy link
Owner

@AustEcon AustEcon Dec 12, 2020

Choose a reason for hiding this comment

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

I think this should be:

        r = requests.post(
            cls.MAIN_TX_PUSH_API,
            data=rawtx
        )

otherwise it blows up 😃

if page == response['totalPages']:
break
page += 1

Copy link
Owner

Choose a reason for hiding this comment

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

I am worried that having this as a generator leads to an inconsistency in the return value when using the NetworkAPI (which requires all subordinate apis to have a consistent / normalized return value)

Copy link
Owner

@AustEcon AustEcon Dec 12, 2020

Choose a reason for hiding this comment

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

I am guessing you want to use it as an iterator over a bitcom namespace or something... which is a valid use case but the other apis don't give back paging information so maybe there needs to be two separate versions of this function - 1 as a generator and the other for plugging into the standard normalized get_transactions call.

Open to suggestions but NetworkAPI needs to have a consistent return type no matter which underlying api it is using to get the job done.

adding get_transactions_gen and normalizing get_transactions to return a single list is just the first idea that comes into my mind..

Copy link
Owner

Choose a reason for hiding this comment

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

Or alternatively could look at WhatsonchainNormalised and do the same thing for BSVBookGuardaAPI to keep the generator but unroll it into a list in BSVBookGuardaAPINormalised.get_transactions

Copy link
Contributor

Choose a reason for hiding this comment

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

I think get_transactions() should define the order of its results. I suggest newest to oldest, and I hope service implementations that support paging would be intelligent enough to match this ordering for efficiency.

Then, add a keyword arg to control the maximum number of results: get_transactions(*, n=10). The hope is that the default of 10 would be less that the paging size of any service implementation, and so avoids paging.

For now I'd punt on any paging API. Reconsider when bitsv has a lot more users.

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

Choose a reason for hiding this comment

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

Again... I believe the reason you have added the * symbol here is because adding get_transactions as a generator means that the return value is no longer consistent - what shall we do to keep it consistent?

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.

If the above things are addressed - happy 😃

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

Choose a reason for hiding this comment

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

results[:100] is surely a bug in the existing test, since results items are the result of each API (currently 2 of them)

return r.json()['transactions']
page = 1
while True:
r = requests.get(cls.MAIN_TX_PULL_API.format(address, page), timeout=DEFAULT_TIMEOUT)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should use ?details=txids for efficiency


@classmethod
def get_balance(cls, address):
r = requests.get(cls.MAIN_BALANCE_API.format(address), timeout=DEFAULT_TIMEOUT)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should use ?details=basic for efficiency

r.raise_for_status() # pragma: no cover
utxos = [
Unspent(amount=currency_to_satoshi(utxo['amount'], 'bsv'),
Unspent(amount=currency_to_satoshi(utxo['value'], 'satoshi'),
Copy link
Contributor

Choose a reason for hiding this comment

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

int() cast is sufficient

AustEcon added a commit that referenced this pull request Jan 23, 2021
- (bitsv is not suitable for heavy address reuse into the thousands)
@AustEcon
Copy link
Owner

AustEcon commented Jan 23, 2021

I kind of agree with @kcentrifugal about leaving the paging mechanics and generator expression to one side at the moment

Reasoning: bitsv is not intended to be heavy-duty client software for pulling in thousands of txids (or indexing a bitcom namespace for example). At least, I don't have the time to spend on bitsv right now to allow the scope to creep out.

I have pushed what I think is a reasonable compromise to the above: fca4767

Basically it just always pages through all txids and returns a list of txids (which keeps the return type the same as all other APIs).

This will obviously be very bad for thousands of txids but I don't think that usecase is appropriate anyway (at least I don't have the ability to support it right now and if I did, I wouldn't be using slow, clunky RESTAPIs for it...

Really you'd be looking at a bitsv2.0 async library that hooks into something more like ElectrumX (and whatever services rival it)

But feel free to fork bitsv and make your own way 😃
I do not want to be a blocker for anybody's building - you all have my blessing 🙏

PS: Sorry @xloem that git is not showing you in the commit history for this one... I had to mix in quite a lot of other changes and I don't know how to force dual authorship.. hope you don't mind.

@AustEcon AustEcon closed this Jan 23, 2021
@xloem
Copy link
Contributor Author

xloem commented Jan 23, 2021

should be fine enough. thanks for your efforts @AustEcon. the reason for many txids was to open avenues for implementing the D:// protocol when many BCAT:// files have been uploaded. I infer that electrumx is the way to go for such ideas.

@xloem
Copy link
Contributor Author

xloem commented Jan 23, 2021

note: for polyglot, that might mean porting to use electrumx for general D:// use, if there's no backend interface here that would provide for it

@AustEcon
Copy link
Owner

AustEcon commented Jan 23, 2021

I don't know sorry. I haven't looked at _unwriter's stuff in a long time - been focused more on ElectrumSV wallet which uses ElectrumX currently as the backing indexer and the SDK (https://github.com/electrumsv/electrumsv-sdk), as well as other projects...

But if I were pulling a ton of txids (and txs) from a single heavily-reused address (as a bitcom namespace) or D:// key that has been updated a lot... I'd be reaching for asyncio for concurrent requests.

ElectrumX is my comfort zone but I imagine Bitbus/Bitsocket from _unwriter would have something more tailor made - You'd know better than me though.

@AustEcon
Copy link
Owner

AustEcon commented Jan 23, 2021

I have a chain indexer of my own that is mostly complete that will cover all needs and handle scaling-testnet loads but in the meantime, you may need to forge your own path (with all of the planaria / _unwriter stuff) - I hate being a bottleneck for other people so please don't wait around on me - fork if you need to. Make your own libraries for the _unwriter stuff that complement bitsv... There are so many opportunities and so much building to be done... Don't let me hold you up! 😃

But I'll probably keep bitsv quite minimalist and deal with bug fixes and basic maintenance at least in the near-term (6 - 12 months)

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