-
Notifications
You must be signed in to change notification settings - Fork 1
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 cancel-stuck-transactions
#363
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.
I am concerned about coupling the database schema with the data structure used by the cancellation library. But I guess that's easy to change later.
The rest of the changes LGTM.
Remember to fix the failing tests.
This comment was marked as outdated.
This comment was marked as outdated.
lib/evaluate.js
Outdated
@@ -147,9 +149,10 @@ export const evaluate = async ({ | |||
|
|||
const start = Date.now() | |||
const buckets = createSetScoresBuckets(participants) | |||
let tx | |||
for (const [bucketIndex, bucket] of Object.entries(buckets)) { | |||
try { |
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.
Here is how I understand your new version at the high level:
let tx
for (const [bucketIndex, bucket] of Object.entries(buckets)) {
tx = await ieContractWithSigner.setScores // etc.
}
tx.wait()
This code will wait only for the last transaction created.
If that's intentional, then let's make it more obvious to the reader, please.
For example:
let lastTx
for (const [bucketIndex, bucket] of Object.entries(buckets)) {
const tx = await ieContractWithSigner.setScores // etc.
lastTx = tx
}
// add a comment explaining why we wait for the last transaction only
// and don't need to wait for other transactions
lastTx.wait()
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.
This was even more broken. Each tx should be awaited and then marked as confirmed. 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.
It may be worth creating a test to verify this works as intended. However, mocking all the dependencies to simulate two TXs running concurrently will require quite a bit of code, so it may not be the best use of our time right now.
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.
👌🏻
Will now mirror this to |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Strange! In the example script of
I'll either add retry logic or let it fail more silently (the whole op will be retried anyway) |
I think I know what happens: When reading from postgres, it doesn't return a BigInt |
No description provided.