Skip to content

Conversation

@Maikol
Copy link
Member

@Maikol Maikol commented Dec 15, 2025

No description provided.

@Maikol Maikol force-pushed the mde/ci-release-tags branch from 369f829 to 37ae754 Compare December 15, 2025 22:22
@Maikol Maikol force-pushed the mde/dataedge-config-posting branch from 5691a31 to 667f062 Compare December 15, 2025 22:22
@MoonBoi9001
Copy link
Member

Minor: signing key parsed twice

The signing key gets parsed twice in the non-dry-run path due to scoping — once for DataEdge posting, then again for StateManager setup. Not a bug, but could be cleaner:

async fn run(logger: Logger, config: Config) -> Result<()> {
    let config_params = /* ... */;

    // Parse signing key once at the top of the non-dry-run path
    let signing_key: Option<SecretKey> = if config.dry_run {
        None
    } else {
        Some(config.signing_key.as_ref().unwrap().parse()?)
    };

    if config.dry_run {
        // dry-run logic...
    } else {
        let signing_key = signing_key.as_ref().unwrap();
        // DataEdge posting using signing_key...
    }

    // ... ipfs, subgraph setup ...

    let contract: Box<dyn StateManager> = if config.dry_run {
        Box::new(StateManagerDryRun::new(logger.clone()))
    } else {
        let signing_key = signing_key.as_ref().unwrap();
        state_manager(config.url, signing_key, /* ... */).await?
    };
    // ...
}

Low priority, just a cleanup opportunity.


Review by @MoonBoi9001 with @claude

@RembrandtK
Copy link

I see there are two versions of post_config_if_changed, for dry run and not, meaning two versions to maintain and reduced confidence (and scope of coverage I see from calling context). I would be tempted to refactor, perhaps a fn that determines if there was a change. It could return enum for detected change (but not post), and then if changed can if one place decide if posting or doing dry run. It might also lead to clean separation of change determination from logging about it. I see calling context is quite different though, not looked through to see if easy to adjust.

We have no safety limit on gas price? Or do we limit by how much is available to spend?

Minor, but you can switch to format! (runtime allocation) to a compile time constant using const VERSION: &str = concat!("v", env!("CARGO_PKG_VERSION"));. You would end up calling .to_string() on it though unless you change the struct type too, which would limited the already minimal gain.

Base automatically changed from mde/ci-release-tags to main December 16, 2025 13:00
@Maikol
Copy link
Member Author

Maikol commented Dec 16, 2025

Thank you for the reviews.

@MoonBoi9001 addressed your suggestiong on b2b6813

@RembrandtK implemented your suggestion to have a separate check_config to have the comparison in a single implementation dfd818c

We have no safety limit on gas price? Or do we limit by how much is available to spend?

We don't have a limit on gas price, I just followed what we are doing on the other transactions that are sent from this oracle. We might want to add a limit eventually but we also need to think what the behavior should be for high gas price.

Minor, but you can switch to format! (runtime allocation) to a compile time constant using const VERSION: &str = concat!("v", env!("CARGO_PKG_VERSION"));. You would end up calling .to_string() on it though unless you change the struct type too, which would limited the already minimal gain.

I left this as it is, the gain is small and this is a one time startup operation.

@Maikol Maikol merged commit 901fb23 into main Dec 16, 2025
3 checks passed
@Maikol Maikol deleted the mde/dataedge-config-posting branch December 16, 2025 19: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.

4 participants