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

Feature/denunciation reloaded #3666

Merged
merged 13 commits into from
Mar 10, 2023
Merged

Conversation

sydhds
Copy link
Contributor

@sydhds sydhds commented Mar 9, 2023

  • document all added functions
  • try in sandbox /simulation/labnet
  • unit tests on the added/changed features
    • make tests compile
    • make tests pass
  • add logs allowing easy debugging in case the changes caused problems
  • if the API has changed, update the API specification

@sydhds sydhds marked this pull request as ready for review March 9, 2023 08:35
@sydhds sydhds requested review from bilboquet and damip March 9, 2023 08:41
@damip
Copy link
Member

damip commented Mar 10, 2023

I find it a bit weird to use a Serializer for DenunciationData because:

  • it doesn't really serialize (for example the variant number of the enum is not encoded)
  • it can return an Error which complicates everything upstream, but compute_hash does not need to return a Result

This can be simplified by having two strucutres (we don't need an enum there, does not serve any genericity purpose anyway):

  • EndorsementDenunciationData with public fields and a to_bytes() method
  • BlockHeaderDenunciationData with public fields and a to_bytes() method

I would also define the EndorsementDenunciationData within the endorsement.rs file to avoid circular dependencies between denunciation.rs and endorsement.rs. Would do the same for the block header.

What do you think ?

@sydhds
Copy link
Contributor Author

sydhds commented Mar 10, 2023

I find it a bit weird to use a Serializer for DenunciationData because:

  • it doesn't really serialize (for example the variant number of the enum is not encoded)
  • it can return an Error which complicates everything upstream, but compute_hash does not need to return a Result

This can be simplified by having two strucutres (we don't need an enum there, does not serve any genericity purpose anyway):

  • EndorsementDenunciationData with public fields and a to_bytes() method
  • BlockHeaderDenunciationData with public fields and a to_bytes() method

I would also define the EndorsementDenunciationData within the endorsement.rs file to avoid circular dependencies between denunciation.rs and endorsement.rs. Would do the same for the block header.

What do you think ?

Done

@sydhds sydhds merged commit 14f78b4 into testnet_21 Mar 10, 2023
@sydhds sydhds deleted the feature/denunciation_reloaded branch March 10, 2023 15:51
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.

4 participants