Closed eadsoy closed 2 years ago
@eadsoy I think either the code here is wrong, or the comment is wrong, or both. I think nanoseconds are seconds * 1B.
Yes, the comment was wrong. I corrected it in the next commit e705599, https://github.com/NEAR-Edu/near-certification-tools/blob/e7055994ce471ba953008416fafed5d10e743f1b/web-app/helpers/expiration-date.ts#L77
Previously issued_at
was coming in as a date string, now it's again a string but a UNIX timestamp in milliseconds. It's already in the format we want, so there is no need to convert it. That is why I removed the dayjs conversion.
Approved. But I am wondering if we tried to use Prisma ORM code instead of using the raw sql approach ? One downside of using raw sql that it opens us to sql injection attacks. It's not a big deal in this case, because we are working with a read only database.
I'll share a gist to an example I was working on using Prisma ORM code, but I do not think we should block this PR. We have 6 months to decide if we want to refactor anything, right?
https://www.loom.com/share/d87f4807646c4b79b2f5eae3ab4e2879
Here is the gist: https://gist.github.com/sccheruku/94f0d1406c3ebace3de4045c095eef9f (on testnet, my query took about 300ms when I set the minimum activity period to 7 days instead of 180 days)
It's a first iteration of the code, so it does not tackle all the edge cases @eadsoy covered in the tests.
@sccheruku Thanks for the review, the gist, and the video. I've used Prisma before but have definitely run into limitations, and I'm not 100% sure it can handle what we're trying to do.
If we can use Prisma, then I agree that we should seriously consider it. Abstracting away from raw SQL can provide some flexibility (allows us to swap out the database underneath, theoretically).
I might be misunderstanding your approach, but something doesn't feel ideal about iterating an indeterminately large number of times to check for each period (currently defined as 6 months, but that's an arbitrary number that might change int he future too).
We can talk about it live today.
Looks like the test script is picking up the correct db but the migration is failing