Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

feat: add dependencies to MultiBindingsInner #2606

Merged
merged 3 commits into from
Oct 10, 2023

Conversation

Autoparallel
Copy link
Contributor

@Autoparallel Autoparallel commented Sep 19, 2023

Motivation

This seeks to close #2599 and should link to Foundry PR #5836. The intent is to be able to allow for serde::{Deserialize, Serialize} to be derived on contract bindings either via module or crate.

PR here has the intent of providing support for an additional dependency in the Cargo.toml generated when bindings are output as a crate.

Solution

This solution here seems a bit simplistic and forcefully assumes the end user will want a specific version of serde as an included dependency. It could be good, instead, to:

  • Allow for dependencies to be chosen if there are extra derives added to the crate.
  • Also forcefully add the serde derives to the contract bindings at this level.
  • Check for most recent serde version or the version used within ethers-rs directly.

Let me know if we wish to make any of the above changes or otherwise!

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

almost there :)

root,
bindings,
shared_types,
dependencies: vec![String::from(r#"serde = "1.0.188""#)],
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm I don't think we serde by default here

what we need is a setter function just like MultiBindings::format

/// Specify whether to format the code or not. True by default.
///
/// This will use [`prettyplease`], so the resulting formatted code **will not** be affected by
/// the local `rustfmt` version or config.
pub fn format(mut self, format: bool) -> Self {
self.format = format;
self
}

so that this can be set manually:

MultiBindings::dependencies(impl IntoIter<impl Into<String>) -> Self

@Autoparallel
Copy link
Contributor Author

@mattsse you think this is good a enough approach?

I'll fix linter issues. Any tests or anything you need me to add here? Doesn't seem breaking!

@mattsse
Copy link
Collaborator

mattsse commented Sep 20, 2023

just want this addressed, then this is good to merge

#2606 (comment)

@Autoparallel
Copy link
Contributor Author

Ah okay I will get to it. My jet lagged brain missed your comment! 😅

@Autoparallel
Copy link
Contributor Author

Alright, I think that does it. I appreciate the help and I see how that externalizes it now and it seems to be working with a slight change in Foundry now too.

@Autoparallel
Copy link
Contributor Author

@mattsse just checking in on this, I just ran the formatter and hoping this passes now.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

paging @DaniPopes

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

really sorry for the delay - good improvement, thank you

@gakonst gakonst merged commit a13233a into gakonst:master Oct 10, 2023
19 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: derive Serialize and Deserialize for Event, {}Filter, Call, and Errors in contract bindings
3 participants