-
Notifications
You must be signed in to change notification settings - Fork 5
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
Insert Settlement & Mark Processed #276
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Left some comments and very nitpicky suggestion.
await db.query(sql`TRUNCATE TABLE settlements;`); | ||
await db.query(sql`TRUNCATE TABLE internalized_imbalances;`); | ||
await db.query(sql`TRUNCATE TABLE settlement_simulations;`); | ||
await db.query(sql`TRUNCATE TABLE tx_receipts;`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nit: Can be wrapped in a for loop for each table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope -- sorry not gonna do this now:
Feel free to try it out if you'd like
https://github.com/cowprotocol/solver-rewards/actions/runs/4946979712/jobs/8845749193
await db.query(sql`TRUNCATE TABLE settlements;`); | ||
await db.query(sql`TRUNCATE TABLE internalized_imbalances;`); | ||
await db.query(sql`TRUNCATE TABLE settlement_simulations;`); | ||
await db.query(sql`TRUNCATE TABLE tx_receipts;`); | ||
await truncateTables(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this! Good job on reducing duplicated code.
test("markReceiptProcessed does nothing when hash doesn't exist", async () => { | ||
await expect( | ||
markReceiptProcessed(db, "0x01") | ||
).resolves.not.toThrowError(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity: Should we ever be expecting to mark a receipt as processed when there isn't a hash for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in the scenario where we run the program manually to "backfill" the data from many months ago till now, we will be calling this program on records that were never part of the "tx_receipts" table.
This reverts commit de3d915.
Part of #177
Doing a TODO to mark data as process when simulation is inserted. This happens in both cases of the pipeline (when there is an is not a simulation to go with it).
Test Plan
Every component is tested individually. Could also add a test fot
insertSettlementAndMarkProcessed
although it just runs both, tested methods, together.