-
Notifications
You must be signed in to change notification settings - Fork 19
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 chain id to bus messages #4409
Conversation
@@ -78,6 +78,7 @@ type App struct { | |||
txTotals []uint64 | |||
txSizes []int | |||
cBlock string | |||
chainCtx context.Context // use this to have access to chain ID |
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.
Minor: we probably don't need chainCtx
if we just did something like this in OnBeginBlock
? I could be wrong
ctx = vgcontext.WithTraceID(app.chainCtx, hash)
ctx = vgcontext.WithBlockHeight(ctx, req.Header.Height)
if chainID, err := vgcontext.ChainIDFromContext(app.blockCtx); err != nil {
ctx = vgcontext.WithChainID(ctx, chainID)
}
app.blockCtx = ctx
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 actually wrote it like first, but couldn't quite convince myself that app.blockCtx was always going to be one that already has a chain id in it. It seemed like it probably would :) Happy to change it to this if you think it's better that way. I think perhaps I'm missing something but contexts seem a bit suspect to me, you never quite know what is going to be in 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.
Nah, don't bother changing it if you have doubts about it. You've probably thought about it harder than I have!
I'm guessing the change to core needs to be merged first else it'll all fall apart? |
closes #4408 (or will once it's done)