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.

Sign up for GitHub

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

Jump to bottom

Truffle Decoupling #117

Merged
merged 11 commits into from
Jan 14, 2020
Merged

Truffle Decoupling #117

merged 11 commits into from
Jan 14, 2020

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented Jan 9, 2020

This PR decouples the deployment futures from truffle. This accomplishes two things:

Test Plan

CI - This is mostly a refactoring and adding new abstractions

@nlordell nlordell marked this pull request as ready for review January 13, 2020 15:24
@nlordell nlordell requested review from fleupold and bh2smith January 13, 2020 15:32
fleupold
fleupold approved these changes Jan 14, 2020
View reviewed changes
Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

Looks good, although I don't 100% understand how you broke the truffle dependency. You basically lifted the ABI module into common namespace, right?

src/contract.rs Outdated
/// `Deployments` implementation for an `Instance`. This type is not intended to
/// be used directly but rather through the `Instance::deployed` API.
#[derive(Debug, Clone)]
pub struct Networks {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this name a bit confusing. Why would a network have a field called ABI? Maybe Deployments is a better name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely a better name!

src/contract.rs
use crate::truffle::abi::Result as AbiResult;
use crate::truffle::{Abi, Artifact, Bytecode};
use ethcontract_common::abi::Result as AbiResult;
use ethcontract_common::truffle::Network;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still truffle specific or should this also be lifted into common?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is truffle specific.

@nlordell
Copy link
Contributor Author

@fleupold So, we are still very much dependent on Truffle artifacts for generating code. The important decoupling is that deployment (so DeployedFuture and Deploy{Builder,Future}) no longer depend on a truffle Artifact. This allows the generated code to slowly add support for things outside of truffle for example:

  • Custom networks definitions (for the dev networks in dex-services for example so we can use pre-built contracts from the npm package).
  • Support for other formats like ethPM and maybe getting verified contracts directly from etherscan.io

While the Instance type still depends on Truffle artifacts, the building blocks for it no longer do so we can start generating code that doesn't as well.

@nlordell nlordell merged commit 6e62a99 into master Jan 14, 2020
@nlordell nlordell deleted the truffle_decoupling branch January 15, 2020 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fleupold fleupold fleupold approved these changes

@bh2smith bh2smith Awaiting requested review from bh2smith

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nlordell @fleupold