-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add idempotency key support #244
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
Conversation
4b3d0b1 to
fa9f4fe
Compare
…on processing Updates aggregator database schema, API validation, and storage layer to use idempotency_key for deduplication instead of verification_timestamp. Modifies verifier pipeline to generate and propagate idempotency keys through the CCVDataWithIdempotencyKey paired struct from source readers to storage operations.
fa9f4fe to
2719f1e
Compare
…ntractkit/chainlink-ccv into simon/aggregator/idempotency-key
| signature_s BYTEA NOT NULL DEFAULT '', | ||
| ccv_node_data BYTEA NOT NULL, | ||
| verification_timestamp BIGINT NOT NULL, | ||
| idempotency_key TEXT NOT NULL, |
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.
Any reason for not using UUID? It's more efficient, less storage and perfectly fit for an idempotency key
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.
The main reason was not to enforce any format on the client. When storing as text a verifier could use any value they see fit as their idempotency key.
It's a question of whether we want to be more flexible or optimize storage efficiency.
I like that we let the caller pick the idempotency value without any strong validation the aggregator side, but open to change it if we think it's better to enforce the UUID as idempotency key
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 do think it's not wise to let the client choose the idempotency key freely. It's error prone to do so IMO.
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.
and uuid are global enough, easy to use and battle tested already. On top of it the efficiency and the more performant storage. I think it's a clear choice.
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 do the change just that I think it's not necessarily a clear cut decision, probably it's pretty low stakes in our context anyways:
https://sdk.amazonaws.com/swift/api/awsec2/1.5.74/documentation/awsec2/runinstancesinput/clienttoken
We don't have to do anything with the key so if the client wants to use timestamp as value they could. But I guess they could derive a UUID from any value they want.
E2E Smoke Test Results
Full logs are available in the workflow artifacts. |
|
Code coverage report:
|
feat: add idempotency key support to prevent duplicate CCV verification processing
Updates aggregator database schema, API validation, and storage layer to use idempotency_key for deduplication instead of verification_timestamp. Modifies verifier pipeline to generate and propagate idempotency keys through the CCVDataWithIdempotencyKey paired struct from source readers to storage operations.