Skip to content

Conversation

@jaoleal
Copy link
Collaborator

@jaoleal jaoleal commented Dec 18, 2024

While searching a way to implement MTP, i got myself a lot of the times with the UtreexoNode declaration aside my readings trough florestad`s internals just because

pub struct UtreexoNode<T, Chain>(
 NodeCommon<Chain>,
 T
)

The generic trait declaration is inverted and the fields had no name, usually being called as self.0 or self.1 which is not so intuitive to look at.

UtreexoNode Signature is UtreexoNode<T,Chain> but to make chain operations in (e.g. access NodeCommon) you had to use self.0

IMO is better to always name internal fields if they're being accessed from outside the struct and not only from internal methods for that structure

@Davidson-Souza
Copy link
Member

I like the idea. Just don't really like core and node_module. I'm leaning towards common and context

@Davidson-Souza Davidson-Souza added documentation Improvements or additions to documentation code quality Generally improves code readability and maintainability labels Dec 18, 2024
@jaoleal jaoleal force-pushed the better_naming_on_utreexonode branch from f6d4c1f to d7d5f6f Compare December 19, 2024 20:08
@jaoleal
Copy link
Collaborator Author

jaoleal commented Dec 19, 2024

I dont like context so much... i had to spend 2 entire hours reading the code trying to understand what context is...

Since this can be a skill issue of me i implemented the changes and you can merge at it is or i can change again if you decide to.

@jaoleal jaoleal force-pushed the better_naming_on_utreexonode branch from d7d5f6f to 35837dc Compare December 19, 2024 20:11
@jaoleal
Copy link
Collaborator Author

jaoleal commented Dec 19, 2024

Forgot the generic type declaration.

Copy link
Member

@Davidson-Souza Davidson-Souza left a comment

Choose a reason for hiding this comment

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

LGTM. Just one small thing

@jaoleal jaoleal force-pushed the better_naming_on_utreexonode branch from 35837dc to 7f77d16 Compare January 1, 2025 18:39
@jaoleal
Copy link
Collaborator Author

jaoleal commented Jan 1, 2025

Changed the generic name on UtreexoNode

@jaoleal jaoleal force-pushed the better_naming_on_utreexonode branch from 7f77d16 to 3c72c6c Compare January 1, 2025 18:44
@jaoleal
Copy link
Collaborator Author

jaoleal commented Jan 1, 2025

Rebased

@Davidson-Souza
Copy link
Member

There's a config with rates/floresta-wire/src/p2p_wire/sync_node.rs

@jaoleal
Copy link
Collaborator Author

jaoleal commented Jan 3, 2025

Yes, i was waiting for #310 to get merged

@jaoleal jaoleal force-pushed the better_naming_on_utreexonode branch from 3c72c6c to f2e2e36 Compare January 3, 2025 13:57
Copy link
Member

@Davidson-Souza Davidson-Souza left a comment

Choose a reason for hiding this comment

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

ACK f2e2e36

@Davidson-Souza Davidson-Souza merged commit b304821 into vinteumorg:master Jan 3, 2025
6 checks passed
@jaoleal jaoleal deleted the better_naming_on_utreexonode branch January 3, 2025 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code quality Generally improves code readability and maintainability documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants