Skip to content

Fix compact block issue#1477

Merged
nazar-pc merged 4 commits intomainfrom
rsub/compact-block-regression
Jun 2, 2023
Merged

Fix compact block issue#1477
nazar-pc merged 4 commits intomainfrom
rsub/compact-block-regression

Conversation

@rahulksnv
Copy link
Copy Markdown
Contributor

Since all the messages hit the same server end point, they need to be wrapped in a common enum. The changes from the last PR did not follow this requirement which causes decode failure. This PR fixes the issue

Code contributor checklist:

Copy link
Copy Markdown
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

The explanation of the PR doesn't fully make sense to me. Yes, it needs to be wrapped and I changed the code to do that explicitly. Looks like I forgot ServerMessage::ProtocolRequest in one place? Then the fix would be a sincle line change rather than introduction of more generics and extra impls.

@rahulksnv
Copy link
Copy Markdown
Contributor Author

Then the fix would be a sincle line change rather than introduction of more generics and extra impls.

Not quite .. (I had explained this in more detail in the original PR probably). So we don't want the protocol layer to be aware of the ServerMessage which is consensus specific. For example, we want to be able to reuse the protocol layer as is for execution, and other users.

This would be the simplest fix, than having more complex adapters full of generics, etc

@nazar-pc
Copy link
Copy Markdown
Member

So we don't want the protocol layer to be aware of the ServerMessage which is consensus specific.

But you do have explicit impl that converts into ServerMessage. And because of that you have to add extra generics that would not be there otherwise. I still do not understand how these additional impls and generics are any better than not having them.

@rahulksnv rahulksnv force-pushed the rsub/compact-block-regression branch from bc0ae66 to 758895b Compare June 1, 2023 07:29
@rahulksnv rahulksnv requested a review from nazar-pc June 1, 2023 07:31
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.

2 participants