search

AustEcon / bitsv

BitSV: Bitcoin made easy. Documentation:
https://AustEcon.github.io/bitsv
MIT License
96 stars 28 forks source link

Add bchsvexplorer api for mainnet redundancy only #39

Closed AustEcon closed 5 years ago

AustEcon commented 5 years ago

Here is a possibility to beef up the redundancy only on mainnet.

Please note: The get_transaction() is not normalized to be the same as what BitIndex3 returns... which probably needs fixing before wiring up to NetworkAPI in case people rely on parsing the json object that is returned...

I did this in a hurry on 4hrs sleep. It's unfinished - but maybe we can use this? (just submit more commits to this branch if you want to)

Needs:

ghost commented 5 years ago

I'm curious, why did you do this rather than WhatsOnChain? Does WhatsOnChain not work?

More APIs the better either way.

ghost commented 5 years ago

I've been thinking about the normalizing. I think maybe we should do that in the module itself. If we need access to the base line API calls, they can be extras or _raw, something like that.

AustEcon commented 5 years ago

https://github.com/AustEcon/bitsv/pull/39#issuecomment-513482734

Because UTXO doesn't return number of confirmations from Whatsonchain (they said it's because it uses ElectrumX server for that particular endpoint). That's what I'm waiting on from them... they said they could change the whole thing to provide it but would be ++ work and I don't want to be pushy because bitsv is not the only project in BSV...

Whereas this one ^^ works right now (but I suspect it won't be as robust as BitIndex or WOC when the big blocks come... but for events like what just happened, it will be perfect so still some value to adding it)

Edit: Question re Whatsonchain:

I think we'd get latest block height first then if there's a new block inbetween the two calls it doesn't matter really because the utxo is stamped with a block height so I think we can work out what happened with the most up to date info (block height of utxo). It will just be slower right?... is there a possibility of wrong info because of two function calls? I think we can write a v. simple algorithm that detects this edge case of when a utxo has higher block height than what the latest block height is supposed to be...

Im now thinking this solution will be okay. If you agree I can add whatsonchain when I get time with this modification. (in fact this is essentially what the electrum sv wallet does for transactions).

ghost commented 5 years ago

Interesting, thanks for explaining. I think I'd rather give this one a try now. Hopefully they can fix it by then, or we can add that feature in ourselves.

AustEcon commented 5 years ago

https://github.com/AustEcon/bitsv/pull/39#pullrequestreview-264518301

Yep. I think that's a good design decision to have our network/services folder for "normalized apis" only (so we should instead do the normalization logic internally like you have proposed... and if more modularity is wanted (e.g. for people who only want the raw API) we can have separate repos for each of them on pypi that can be raw... but for the purposes of bitsv, it makes sense to have our internal / built-in APIs customized for use in NetworkAPI).

The other reason it is a good idea to go this route is that IF people DO access these APIs directly (via network/services) then it is probably good to have the return value kept uniform across our in-house APIs.

And as to merging this as-is... I will see what I can do later tonight... But maybe I'll do some bare minimum work and then just merge so you can then take it forward from there.

And re: Whatsonchain. That's cool. We can leave it out for now. But at least we've discussed it so we know that's an option for us if we ever need (and would work across main/test and stn + I predict would be more robust for big blocks etc.. come July 24th...).

PS: With the logging... I think we should copy the electrum sv model for example see: https://github.com/electrumsv/electrumsv/blob/master/electrumsv/storage.py, I like that workflow. But I won't be able to do that tonight so I'll probably just delete the print statement and leave the logger as an attribute. But let it be known: I want it gone!

AustEcon commented 5 years ago

Sorry I am not okay with merging this yet... this is uncovering other things that need to be addressed and I don't want any of the things to be forgetten about later now that I have found them... I'm now into the danger zone with sleep. So I'll have to leave it here. TTFN

ghost commented 5 years ago

Ok, sounds good! Hope you rest well, take your time. Let me know if you want me to jump in on this at any time.

AustEcon commented 5 years ago

Still not done. But you will now see where I'm looking to take it... The annoying thing is that the inputs section of BitIndex3 (and Whatsonchain too I think) don't provide a field that tells you the total amount in the input. Would have been nice to have because then the repr can show:

But I think this would require a second lookup for the prev txid --> getting those outputs... So I will probably abandon... and go with common denominator fields of some kind...

I'll also probably end up just scrapping this PR and resubmitting to clean up the commit history... but I'll use this one more as a first draft.

ghost commented 5 years ago

Nice work! I like these changes.

You don't have to do that, I think you can do a git squash rather than opening a new PR.

This can come later, but I do prefer having either endpoint= in functions for the base endpoint, or using the endpoint as a constant. In bitindex we're retyping the same endpoint url again and again which makes it a lot easier to have a typo there. If we're referencing a variable/pseudo constant it'll break at run time.

AustEcon commented 5 years ago

Closing this PR because work is now being continued on #42