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

Chain switch trace events: record old and new SelectView #384

Merged
merged 2 commits into from
Oct 2, 2023

Conversation

amesgen
Copy link
Member

@amesgen amesgen commented Oct 2, 2023

No description provided.

@amesgen amesgen requested a review from a team as a code owner October 2, 2023 15:20
@amesgen amesgen force-pushed the amesgen/chain-switch-trace-SelectView branch from 7361fd8 to cd46472 Compare October 2, 2023 15:25
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

I'm not Approving only because (as you hinted in your Slack message), I think the field name is maybe a little too confusing.

@amesgen amesgen force-pushed the amesgen/chain-switch-trace-SelectView branch from cd46472 to 44c503b Compare October 2, 2023 17:35
@amesgen amesgen force-pushed the amesgen/chain-switch-trace-SelectView branch from 44c503b to 7458090 Compare October 2, 2023 17:38
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

Nice 👍

@@ -636,7 +649,7 @@ data TraceAddBlockEvent blk =
-- chain (second fragment).
| AddedToCurrentChain
[LedgerEvent blk]
(NewTipInfo blk)
(SelectionChangedInfo blk)
Copy link
Contributor

@nfrisby nfrisby Oct 2, 2023

Choose a reason for hiding this comment

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

Once this diff context appear it occurred to me that perhaps the dowstream use cases of NewTipInfo/SelectionChangedInfo could just get the SelectView from the two header chain arguments.

However, Esgen pointed out the call site would then need to also have the BlockConfig in scope. (That's certainly always possible, but likely often onerous).

@amesgen amesgen enabled auto-merge October 2, 2023 17:42
@amesgen amesgen added this pull request to the merge queue Oct 2, 2023
Merged via the queue into main with commit c6498b6 Oct 2, 2023
12 checks passed
@amesgen amesgen deleted the amesgen/chain-switch-trace-SelectView branch October 2, 2023 18:52
@amesgen amesgen mentioned this pull request Oct 4, 2023
github-merge-queue bot pushed a commit that referenced this pull request Oct 4, 2023
Needed to actually make use of #384 in node.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants