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

Unify retention index type #195

Merged

Conversation

matejpavlovic
Copy link
Contributor

Having a separate type for the retention index for each module
seems like over-engineering, given that they are all used
exactly the same way.
I used the IDE to automatically rename everything.

@matejpavlovic matejpavlovic changed the title Unify retention index Unify retention index type Aug 19, 2022
Copy link
Contributor

@xosmig xosmig left a comment

Choose a reason for hiding this comment

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

Eventually, it would be nice to get rid of the types and events packages or only keep really universal things there, like ModuleID and NodeID. In other words, the core should not know anything about what modules there are or how they are implemented.

That being said, since we are probably not going to get rid of types any time soon, I don't mind this PR being merged.

pkg/types/types.go Outdated Show resolved Hide resolved
@xosmig
Copy link
Contributor

xosmig commented Aug 19, 2022

If there are some repetitive patterns that we want to reuse across modules, we could have a moduleutils package.

Having a separate type for the retention index for each module
seems like over-engineering, given that they are all used
exactly the same way.

Signed-off-by: Matej Pavlovic <matopavlovic@gmail.com>
@matejpavlovic
Copy link
Contributor Author

matejpavlovic commented Aug 22, 2022

Eventually, it would be nice to get rid of the types and events packages or only keep really universal things there, like ModuleID and NodeID.

Yes I absolutely agree. Things like PBFTViewNr have nothing to do there. It's a relict from times where Mir was not as general as it is now.

If there are some repetitive patterns that we want to reuse across modules, we could have a moduleutils package.

Yes that is definitely a good direction. I'm creating an issue so I don't forget to clean this up.

@matejpavlovic matejpavlovic merged commit 8ad8634 into consensus-shipyard:main Aug 22, 2022
@matejpavlovic matejpavlovic deleted the unify-retention-index branch August 22, 2022 13:54
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