Skip to content
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

chore(ckbtc): add optional timestamp to minter events #3157

Merged
merged 6 commits into from
Dec 19, 2024

Conversation

lpahlavi
Copy link
Contributor

@lpahlavi lpahlavi commented Dec 12, 2024

(XC-238) ckBTC events, in contrast with ckETH events, do not have a timestamp. This makes it almost impossible to determine when something happened. To address this, this PR adds a timestamp to ckBTC minter events.

In order for this change to be backwards compatible, the timestamp is optional and events logged prior to the introduction of timestamps have a None value for this field. The deserialization of both legacy and new events is supported, however there is a breaking change to the Candid interface of the get_events endpoint of the ckBTC minter (hence the CI_OVERRIDE_DIDC_CHECK label) due to the new structure of the Event type (i.e. addition of a timestamp field). This is acceptable since it is a debug endpoint.

@github-actions github-actions bot added the chore label Dec 12, 2024
@lpahlavi lpahlavi added CI_OVERRIDE_DIDC_CHECK Skips the backwards compatibility didc check (explain in PR description why) chore CI_ALL_BAZEL_TARGETS Runs all bazel targets and uploads them to S3 and removed chore labels Dec 12, 2024
@lpahlavi lpahlavi force-pushed the lpahlavi/XC-147-add-timestamp-to-ckbtc-minter-events branch 4 times, most recently from da59342 to 50dcfc1 Compare December 17, 2024 09:43
@lpahlavi lpahlavi force-pushed the lpahlavi/XC-147-add-timestamp-to-ckbtc-minter-events branch from 50dcfc1 to c4eda9e Compare December 17, 2024 10:38
@lpahlavi lpahlavi marked this pull request as ready for review December 17, 2024 11:06
@lpahlavi lpahlavi requested a review from a team as a code owner December 17, 2024 11:06
@lpahlavi
Copy link
Contributor Author

@ninegua Thanks a lot for the review! I have addressed your feedback and pushed the updated code.

Copy link
Member

@gregorydemay gregorydemay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lpahlavi for this PR, great job handling Candid/serde! Only minor comments a follow-up but otherwise LGTM!

rs/bitcoin/ckbtc/minter/ckbtc_minter.did Show resolved Hide resolved
rs/bitcoin/ckbtc/minter/src/state/eventlog.rs Show resolved Hide resolved
rs/bitcoin/ckbtc/minter/src/storage/tests.rs Show resolved Hide resolved
rs/bitcoin/ckbtc/minter/tests/replay_events.rs Outdated Show resolved Hide resolved
rs/bitcoin/ckbtc/minter/src/updates/tests.rs Outdated Show resolved Hide resolved
@lpahlavi lpahlavi added this pull request to the merge queue Dec 19, 2024
Merged via the queue into master with commit 9afadc4 Dec 19, 2024
25 checks passed
@lpahlavi lpahlavi deleted the lpahlavi/XC-147-add-timestamp-to-ckbtc-minter-events branch December 19, 2024 14:40
github-merge-queue bot pushed a commit that referenced this pull request Dec 23, 2024
…ion (#3277)

[(XC-238)](https://dfinity.atlassian.net/browse/XC-238) Add property
tests for the ckBTC event serialization/deserialization logic where
events (both legacy and new) are arbitrarily generated from all possible
event types. See [this
comment](#3157 (comment)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore CI_ALL_BAZEL_TARGETS Runs all bazel targets and uploads them to S3 CI_OVERRIDE_DIDC_CHECK Skips the backwards compatibility didc check (explain in PR description why) @cross-chain-team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants