Skip to content

Refactor: Move parsing into its own module #214

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

Closed
carlaKC opened this issue Jan 27, 2025 · 4 comments · Fixed by #232
Closed

Refactor: Move parsing into its own module #214

carlaKC opened this issue Jan 27, 2025 · 4 comments · Fixed by #232
Labels
Code Health Refactors and improvements to the structure of the code good first issue Good for newcomers
Milestone

Comments

@carlaKC
Copy link
Contributor

carlaKC commented Jan 27, 2025

As is: we have a monolith main function that performs all parsing and creates a simulation.

Motivation: if using simln as a library, it would be nice to have a parsing module in sim-cli/src/parsing.rs to pull in some of the parsing/validation that main currently performs.

Some suggestions for things that we can pull out in a refactor:

  • Cli: move into module so that it can be re-used externally
  • SimParams: move to parsing module (it doesn't belong in the lib) (also move NodeConnection and ActivityParser)
  • get_clients: builds the map of dyn LightningNode that'll be passed to the simulation
  • validate_activities: performs ActivityParser -> ActivityDefinition conversion
  • read_sim_path: this is a nice to have frill, might as well make it accessible for other folks

The only thing that's tricky here is that validate_activities needs to make calls to our dyn LightningNode to validate that pubkeys exist in their graph, so that needs to be threaded through. Ideally this could just be a generic closure that looks up the node, but it gets ugly when you start dealing with async closures (I briefly tried this in the past and gave up).

@carlaKC carlaKC added Code Health Refactors and improvements to the structure of the code good first issue Good for newcomers labels Jan 27, 2025
@carlaKC carlaKC modified the milestone: V2.4 Feb 12, 2025
@carlaKC carlaKC added this to the V2.4 milestone Mar 7, 2025
@chuksys
Copy link
Contributor

chuksys commented Mar 9, 2025

I've been looking into this issue and noticed there's already some work started on the review-club-memleak branch in your personal fork. Would you recommend that I continue from the work done on that branch, or would you prefer that I start from scratch? I want to make sure I'm contributing in the most helpful way possible. Thanks.

@carlaKC
Copy link
Contributor Author

carlaKC commented Mar 11, 2025

Feel free to use the commits on that branch as a starting point (they're here unsquashed), but I think it would be good to take a look at it with fresh eyes - I did that in a rush for another project.

@f3r10
Copy link
Collaborator

f3r10 commented Mar 15, 2025

Hi @chuksys, I had to complete this PR since I am working on #215. Let me know if you have already made some commits. Maybe we could collaborate on this PR.

@chuksys
Copy link
Contributor

chuksys commented Mar 16, 2025

Hi @f3r10. Yes I have. We can def collaborate on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Health Refactors and improvements to the structure of the code good first issue Good for newcomers
Projects
None yet
3 participants