More Validations And Fixes For Endpoint Return Types

[apbendi]

Validates addresses, poll options, attestation uniqueness, and that the end date is in the future, per #9.

Fixes #23 by returning all dates as integer seconds since Unix epoch. Also returns all foreign Id arrays as integers.

To run, after cloning:

npm run reset-dev-db # Drops & recreates databases to avoid schema conflicts
npm run migrate
npm test
npm run dev

[mds1]

About to start reviewing this, but question about the rebase here—it looks like the commits got duplicated when rebasing above? Will the commit history look like this when merged to the master, or will just the rebased set show up?

[mds1]

Weird, now it only shows the one set. Perhaps the first time you view the PR it shows both sets to make it clear that there was a rebase?

[mds1]

Pushed a commit to this branch which fixes frontend issues caused by a few changes on this PR

Validate address submissions are properly checksummed

This test is incomplete—right now it only passes because we are passing an Ethereum address with an invalid checksum. If we instead pass an address that is entirely lowercase, the ethers.js function assumes the address is not a checksummed address and converts it to one, and the test fails

Validate poll end date is in the future

What is the purpose of this line, particularly the y2k timestamp: if ("number" !== typeof pollData.end_date || pollData.end_date > y2kInMilliseconds). The function seems to work, I just can't tell what the significance of the timestamp is here.

Good to merge after those two small things!

[apbendi]

Thanks for the review, and thanks for applying those fixes! Both are good questions. Let me tackle them:

If we instead pass an address that is entirely lowercase, the ethers.js function assumes the address is not a checksummed address and converts it to one, and the test fails

The convention in web3js, and apparently in ethers as well, seems to be: "if the user passes a fully lowercase address, assume they know what they're doing and that it's correct, because there's no way to get any info from it otherwise. But if the user passes an address that has any capitals, we assume they meant to checksum it, so we might as well validate that checksum for some extra protection."

I basically am taking the same posture here. So for me, accepting an all lowercase address as de-facto properly formed is intentional. Do you agree that's the approach these libraries tend to take, or am I reading it wrong? Do you think we should fail on a lowercased address and require it to be checksummed? I could be convinced.

What is the purpose of this line, particularly the y2k timestamp

I wanted a way to detect if the user of the API had likely submitted a timestamp in milliseconds rather than in seconds since the epoch. Picking some date reasonably far in the past, taking the timestamp of that date in milliseconds, and checking if the timestamp submitted is greater than that seemed like a reasonable approach. Does that make sense? Any cleaner ways to implement?

I'm going to merge this pending your feedback! Happy to make changes to either of these points in future issues after discussing further.

[mds1]

Regarding checksum—That is good with me! From the test name I just had thought you wanted a lowercase address to count as an invalid checksum.

Regarding the y2k timestamp—Got it, that makes sense. The only alternative I can think of would be to set a maximum end date instead. For example, I can't see a practical reason to make a poll with an end date of year 2100 for example, so we'd instead make sure the timestamp is smaller than year 2100s timestamp. But I don't think one approach is necessarily better than the other, so leaving it as-is works for me.

Thanks!