Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add track-payments.html to report on payments to incident victims #399

Merged
merged 6 commits into from May 7, 2020
Merged

Add track-payments.html to report on payments to incident victims #399

merged 6 commits into from May 7, 2020

Conversation

ghost
Copy link

@ghost ghost commented May 4, 2020

The report is in HTML/pure javascript, it makes a web api call to blockchair.com to obtain the information that is presented.

Resolves bisq-network/admin#77

The report is in HTML/pure javascript, it makes a web api
call to blockchair.com to obtain the information that is
presented.

Resolves bisq-network/admin#77
@cbeams cbeams assigned ghost May 6, 2020
@cbeams cbeams self-requested a review May 6, 2020 09:32
Copy link
Contributor

@cbeams cbeams left a comment

Choose a reason for hiding this comment

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

NACK. @jmacxx, I just made a few review commits, but am unable to push them because your PR doesn't "allow maintainers to make edits". Please check that checkbox. In the meantime, take a look at the track-repayments branch I just pushed, where you'll see my changes. In particular, see commit d863a04, where I update the repayment addresses to their current values from bisq-network/admin#76. After making this change, the call to the blockchair API returns 404. Doing a bit of debugging, I narrowed it down to the following:
Notice how this call, using victim 1's original repayment address returns OK:

$ http head https://api.blockchair.com/bitcoin/dashboards/addresses/1EU2aUQkHQukKbk3t5fmA4quHyMRBupM2U
HTTP/1.1 200 OK

Whereas the same request using victim 1's new repayment address value returns 404:

$ http head https://api.blockchair.com/bitcoin/dashboards/addresses/13sxMq8mTw7CTSqgGiMPfwo6ZDsVYrHLmR
HTTP/1.1 404 Not Found

The difference between these two addresses is that the first has had transactions move through it and the latter has not. It seems that blockchair is treating the latter as "not found" for that reason, which is not what we want here. Could you look into this and see if there's a fix? Thanks.

@ghost
Copy link
Author

ghost commented May 6, 2020

@cbeams The box is/was already checked. Regarding the 404 when all addresses are empty, I'll handle it.

image

The blockchair API will return a 404 when none of the addresses
being queried are present in the blockchain database.  For the
report, in this case we still want to show the information with
0 payments made as it is the initial state.

In case we ever get an error code from blockchair API
we'll display the error message under the report.
@ghost
Copy link
Author

ghost commented May 6, 2020

I've made a fix to handle the 404 return code when addresses are all empty. Feel free to make your changes too, @cbeams. I don't know why you experienced the inability to make changes, as stated earlier the box "allow edits" is and always has been checked.

Copy link
Contributor

@cbeams cbeams left a comment

Choose a reason for hiding this comment

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

ACK.

I've made a fix to handle the 404 return code when addresses are all empty.

Thanks for the fix. I've tested this locally and it works as expected for me, including when one of the addresses does have transactions.

Feel free to make your changes too, @cbeams. I don't know why you experienced the inability to make changes, as stated earlier the box "allow edits" is and always has been checked.

My normal workflow was thrown off because you made these changes on your master branch. Not a big deal, I've now pushed my changes and will merge everything shortly. But in the future, please issue pull requests from a branch dedicated to that PR. This is especially important when you're working in a higher-traffic repository like bisq-network/bisq.

@cbeams
Copy link
Contributor

cbeams commented May 7, 2020

And please do have a look through my review commits, @jmacxx, there are notes in the commit messages that you may find helpful in the future.

@cbeams cbeams merged commit b5414de into bisq-network:master May 7, 2020
@ghost ghost mentioned this pull request May 12, 2020
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.

Implement security incident victim repayment tracking mechanism
1 participant