Merge ChainParams to Consensus #338
Conversation
|
Since this can be a complicated PR to review, ill be commiting any changes and squashing them after tagging this as Ready to Merge |
e106055 to
759247c
Compare
|
759247c applies linting too. I dont think theres more work to be done here. |
| /// The parameters of the chain we are validating, it is usually hardcoded | ||
| /// constants. See [ChainParams] for more information. | ||
| pub parameters: ChainParams, | ||
| pub struct ConsensusParameters { |
There was a problem hiding this comment.
I'm not sure about this struct. I find it nicer to have the validation methods separated from parameters, like core does
There was a problem hiding this comment.
Yes, it's interesting... But I also don't see the point in keeping just one struct if the function of this struct is just to be a wrapper of another one that is mainly used.
Maybe in another code reform we can make this separation... We talked in private about such a reform, I believe that this separation would ideally be addressed in this reform. What do you think?
| ), | ||
| } | ||
| } | ||
| /// Validates the locktime of a transaction input. |
There was a problem hiding this comment.
46dc77e
nit: empty line between the functions, and use the attribute before a comment.
759247c to
10e2cfa
Compare
10e2cfa to
53276a3
Compare
|
This needs rebase. Also there are some functions that needs an empty line between them, like in the |
|
Mmm I agree with @Davidson-Souza that the chain params should be in a different file, as they are right now (and as Core does). I think it is very important to keep the consensus functions and the params isolated in their own files. The |
After noticing that
Consensuswas just a wrapper forChainParamsand the filechain_params.rswas only holding some Dns related methods and the structChainParamsalone, i made some changes to merge them and do some code organization/documentation.Hope this helps to simplify our codebase.