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

Refactor: consolidate forum parameters and confirmation parameters into a single structure. #1290

Merged
merged 12 commits into from
Feb 20, 2025

Conversation

DFINITYManu
Copy link
Contributor

@DFINITYManu DFINITYManu commented Feb 19, 2025

What has happened here:

  • Reduction of cyclic use dependencies.
  • Auth stuff moved to auth.rs.
  • ExecutableCommand stuff moved to ctx module, since (despite the name) it's just a vehicle to create a Ctx for commands to use. I will revise this abstraction later on -- I think it may ultimately not be necessary.
  • Consolidation of everything under lib.rs instead of double importing in main.rs. Removes source of confusion and speeds up compilation.
  • Confirmation options move to their own module confirm.rs to clear up confusion and the import graph.
  • Forum submitter code from forum.rs moves to submitter.rs since it has dual responsibility (submit proposals and manage forum posts) so having it as a separate file adds clarity.
  • Implementations of proposal executors from proposal_executors.rs move to governance.rs for the governance executor and ic_admin.rs for the ic-admin executor, breaking up an important import cycle.

One important change not in the list above:

The SubmissionParameters structure has sprung to existence to include the ForumParameters and ConfirmationModeOptions structs, and is now used when appropriate (in operations that require forum post parameters and can be affected by confirmation modes, e.g., dry run, confirm, unconditional).

These changes, together with prior changes that consolidate forum post management and proposal simulation / execution, make the code more maintainable, more explicit about what is implemented or happening and where it is, and pave the way for imbuing the Proposable types with knowledge about how to deal with the forum based on what the type of Proposable.

Noteworthy in light of the last sentence: now the Submitter type method propose() accepts a ProposalExecution (tied to a Proposable) next to a forum post type. Soon, the type of forum post can be dictated by the ProposalExecution (effectively, the Proposable) itself, which makes sense, as we already have fixed forum post types for different proposals, but this knowledge is currently (incorrectly) hardcoded in an ad-hoc manner throughout various parts of the code.

Important change: we hereby sunset the ability to pass --yes, --no and --forum-post stuff at arbitrary positions in the command line. These are now accepted only after the subcommand name. clap -- or perhaps our use of clap -- does not give us flexibility in parsing these flags anywhere in the command line. I removed error-prone code that tried to guess (with only a moderate chance of success) under certain circumstances what the user meant.

Find most current import graphs attached here:

modules

Not all cycles have been eliminated!

…to a single structure.

The 'SubmissionParameters' structure has sprung to existence to include the `ForumParameters`
and `ConfirmationModeOptions' structs, and is now used when appropriate (in operations that
require forum post parameters and can be affected by confirmation modes, e.g., dry run,
confirm, unconditional).

These changes, together with prior changes that consolidate forum post management and proposal
simulation / execution, make the code more maintainable, more explicit about what is taking
place, and pave the way for imbuing the `Proposable` types with knowledge about how to deal
with the forum based on what the type of `Proposable`.

Noteworthy in light of the last sentence: now the Submitter type method `propose()` accepts
a `ProposalExecution` (tied to a `Proposable`) next to a forum post type.  Soon, the type
of forum post can be dictated by the `ProposalExecution` itself, which makes sense, as we
already have fixed forum post types for different proposals, but this knowledge is currently
(incorrectly) hardcoded in an ad-hoc manner throughout various parts of the code.
Interdependencies between modules were insane.  I have removed many cycles from the `use` graph through the application of `cargo-graphmod`.
@DFINITYManu DFINITYManu marked this pull request as ready for review February 19, 2025 17:45
@DFINITYManu DFINITYManu requested a review from a team as a code owner February 19, 2025 17:45
Copy link
Member

@sasa-tomic sasa-tomic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking on this huge quality of life improvement!

@DFINITYManu DFINITYManu merged commit b9e2e98 into main Feb 20, 2025
5 checks passed
@DFINITYManu DFINITYManu deleted the refactorthree branch February 20, 2025 11:53
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