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: Streamline access shared utilities #108

Merged
merged 3 commits into from
Jan 28, 2022
Merged

refactor: Streamline access shared utilities #108

merged 3 commits into from
Jan 28, 2022

Conversation

scarmuega
Copy link
Member

The utils module includes general-purpose utilities that could potentially be used by more than a single stage. Up to this point, access to the utilities was ad-hoc and internal to the code of each stage. This approach is suboptimal in terms of memory (excessive cloning) and prevents from implementing utilities that share a state between stages.

This PR introduces a new struct Utils that serves as an entry point that follows a singelton pattern, all stages share the same instance via an Arc pointer.

This change affects the stage bootstrapping procedure, which resulted in a lot of changes throughout the codebase. Don't be alarmed, is mostly boilerplate.

@scarmuega scarmuega marked this pull request as ready for review January 27, 2022 21:43
@mark-stopka mark-stopka self-assigned this Jan 27, 2022
@mark-stopka mark-stopka added the enhancement New feature or request label Jan 27, 2022
@mark-stopka mark-stopka added this to the v1.1 milestone Jan 27, 2022
@scarmuega scarmuega requested a review from rvcas January 28, 2022 03:19
@pavlix
Copy link
Contributor

pavlix commented Jan 28, 2022

Won't this complicate the future standalone usage of the EventWriter?

This is what I'm currently doing:

let writer = EventWriter::new(output_tx, None, mapper);

Now it looks like I need to do this:

let writer = EventWriter::new(output_tx, Arc::new(Utils::new(ChainWellKnownInfo::mainnet())), mapper);

Not a huge deal but I'm still interested in getting a rather simple convertor object for simple purposes and more complex confiugration only when it's needed.

This works with the following patch:

diff --git a/src/mapper/prelude.rs b/src/mapper/prelude.rs
index b6bceb2..801017a 100644
--- a/src/mapper/prelude.rs
+++ b/src/mapper/prelude.rs
@@ -95,7 +95,7 @@ pub struct Config {
 }

 #[derive(Clone)]
-pub(crate) struct EventWriter {
+pub struct EventWriter {
     context: EventContext,
     output: StageSender,
     time_provider: Option<NaiveTime>,

Code sample:

https://github.com/txpipe/oura/tree/pavlix-cip15

I'm sorry if the name of the branch doesn't fit your workflow, but it's not even really a feature branch at the moment, rather a scratch branch to keep the examples public.

@rvcas
Copy link
Member

rvcas commented Jan 28, 2022

I'll take a look as soon as I start my day tomorrow.

@scarmuega
Copy link
Member Author

@pavlix thanks for the feedback. I wasn't taking into account that use-case, which I think makes a lot of sense. It could also be used for some blackbox testing of the mapper (putting a known block and expecting a known set of events).

To address your concern, I've added the following constructor to the EventMapper:

pub fn standalone(
        output: StageSender,
        well_known: Option<ChainWellKnownInfo>,
        config: Config,
    ) -> Self {
        let utils = Arc::new(Utils::new(well_known.unwrap_or_default()));

        Self::new(output, utils, config)
    }

In this way, we could create a new one by calling EventMapper::standalone(output, None, config). Does that make sense?

@@ -95,6 +95,12 @@ impl FromStr for MagicArg {
}
}

impl Default for MagicArg {
Copy link
Member

Choose a reason for hiding this comment

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

awesome :)

//!
//! This module includes general-purpose utilities that could potentially be
//! used by more than a single stage. The entry point to this utilities is
//! desgined as singelton [`Utils`] instance shared by all stages through an Arc
Copy link
Member

Choose a reason for hiding this comment

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

typo: designed

@rvcas
Copy link
Member

rvcas commented Jan 28, 2022

This looks good to me. Other than the typo I don't see any problems. I didn't request changes because that typo doesn't affect the code. I didn't press approve because I assume we're waiting for @pavlix to reply to your latest changes :).

src/mapper/prelude.rs Show resolved Hide resolved
src/utils/mod.rs Show resolved Hide resolved
@mark-stopka mark-stopka merged commit 96d7a87 into main Jan 28, 2022
@mark-stopka
Copy link
Collaborator

mark-stopka commented Jan 28, 2022

Merged in now, thank you all contributors!

@mark-stopka mark-stopka deleted the feat/utils branch January 28, 2022 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants