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

CSV address data download. #894

Merged
merged 8 commits into from Dec 21, 2018
Merged

CSV address data download. #894

merged 8 commits into from Dec 21, 2018

Conversation

buck54321
Copy link
Member

@buck54321 buck54321 commented Dec 18, 2018

Resolves #854

Essentially returns rows of the addresses table with CSV-formatting and appropriate headers.
It is surprisingly fast, even for the treasury address which returns a 46 MB file.

The rows returned are

  1. tx_hash
  2. valid_mainchain: [0,1]
  3. matching_tx_hash: can be empty
  4. value: dcrutil.Amount(value).ToCoin()
  5. block_time: uses TimeDef.String() YY-MM-DD HH:MM:SS format, but should maybe be either a timestamp or an ISO formatted string
  6. direction: [1,-1] Indicates whether this is input or output of the associated transaction. +1 for output (increasing address balance), -1 for input.
  7. io_index
  8. tx_type: from txhelpers.TxTypeToString(txType)

Potential areas of discussion

  1. There is no block hash (or height) column, but if it is needed, I could perhaps do a JOIN with the transactions table.
  2. This introduces a new route in apirouter at the /api/download path. There may be an argument for creating a new router at /download instead.
  3. The auto-suggestion for the download filename is not customized in any way. It is always 'address-io.csv'.
  4. Nothing is paginated in any way. You get everything every time. Though as mentioned above, everything seems to run quite fast.

One more note. I'm not sure why the binary icon font files got smaller when I added an icon. All of the icons seem to still be there.

image

Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

I would make time_stamp a unix epoch time stamp, since it is compact and unambiguous.

The PR description has a little copypasta: tx_type comes from txhelpers.TxTypeToString(txType) and value from Coin.

I'd also suggest putting direction either right before or right after matching_tx_hash, and with io_index adjacent to matching_tx_hash too.

I think there's a pretty strong argument for changing /api/download to just /download with a new router, primarily since the content-type is different.

For the download file name, some kind of current time stamp seems appropriate, perhaps with a best block height.

How were the ttf and svg files edited? fontforge and Inkscape? The file size changes are peculiar.

api/apiroutes.go Outdated Show resolved Hide resolved
@chappjc
Copy link
Member

chappjc commented Dec 18, 2018

Oh, it looks like a thumbs up and thumbs down glyph were removed from the ttf.

public/fonts/icomoon.svg Outdated Show resolved Hide resolved
@buck54321
Copy link
Member Author

New header sequence is "tx_hash", "direction", "io_index", "valid_mainchain", "value", "time_stamp", "tx_type", "matching_tx_hash".

Server's filename recommendation is address-io-[best block height]-[hex encoded timestamp].csv.

New path is /download/address/io/{address}.

re: font file stuff. I guess I can't say for sure, but it looks like the fonts were previously created with IcoMoon, so that's what I used too. IcoMoon generates the 4 font files. Unfortunately, this means that I can't tell you why the decimal precision changes, but it looks like it only affects 3 numbers and not the entire SVG. I think the thumbs up/down icons were extraneous because I can't seem to find a reference to them in the code. Perhaps someone previously pruned the icon set but didn't replace the TTF.

@chappjc I didn't change the address validation because I'm pretty sure that it's right, but could you check.

@chappjc
Copy link
Member

chappjc commented Dec 19, 2018

The address validation is good for sure, I just wanted to call out that there are a lot of non-nil error values that are not unexpected. I wanted to be explicit about my motivation for changing to Debugf from Errorf.

api/apiroutes.go Outdated Show resolved Hide resolved
api/apiroutes.go Outdated Show resolved Hide resolved
api/apiroutes.go Outdated Show resolved Hide resolved
views/address.tmpl Outdated Show resolved Hide resolved
public/fonts/icomoon.svg Show resolved Hide resolved
db/dcrpg/internal/addrstmts.go Outdated Show resolved Hide resolved
db/dcrpg/queries.go Outdated Show resolved Hide resolved
db/dcrpg/queries.go Outdated Show resolved Hide resolved
@chappjc chappjc merged commit a595764 into decred:master Dec 21, 2018
@chappjc chappjc added this to the 4.0 milestone Dec 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants