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

feat: add multi-target Typechain typings build option #131

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JDawg287
Copy link
Member

@JDawg287 JDawg287 commented Mar 19, 2024

Description

Reopening the PR that was previously reverted.

Previous PR #127

PR Checklist:

  • I have performed a self-review of my code
  • I have commented on my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added or updated tests that comprehensively prove my change is effective or that my feature works

@JDawg287 JDawg287 self-assigned this Mar 19, 2024
@JDawg287 JDawg287 marked this pull request as ready for review March 19, 2024 16:23
Copy link

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

The code looks fine but I have two questions:

  1. Why do we want to be agnostic about the typechain? Why not be opinioated and only support ethers 6? What do we gain from being flexible here?

  2. Reading thew (great!) docs about how to pass in the preferred typechain, I'd like to know what the priority rules are. Env var is trumped by CLI arg? Both trumped by config file? The downside of great flexibility is that we now must test&document all the permutations. Is the flexibility worth it?

@JDawg287
Copy link
Member Author

To answer the following question:

  1. Reading thew (great!) docs about how to pass in the preferred typechain, I'd like to know what the priority rules are. Env var is trumped by CLI arg? Both trumped by config file? The downside of great flexibility is that we now must test&document all the permutations. Is the flexibility worth it?

I did light testing on my part with all the scenarios. As for the priorities I set up the following:

    const typechainTarget =
      args.typechainTarget ||
      process.env.TYPECHAIN_TARGET ||
      hre.config.typechain.target // default 'ethers-v6'

The command line argument is the highest priority and overrides the rest since it is straightforward.
If there are no command line arguments but an environment variable is present instead, we take the value from there.
If either of them is not present then the value defaults to the hardcoded value in the config file.
Shouldn't be that complicated here.

As for the first question, @sebastiendan might be able to answer that with more detail.

@sebastiendan
Copy link
Member

sebastiendan commented Mar 26, 2024

As for the first question, @sebastiendan might be able to answer that with more detail.

@dvdplm This is related to topos-js and its unbiased architecture (potentially usable with any evm chain client, with any graphql client, etc.)

@sebastiendan sebastiendan removed their request for review April 8, 2024 09:05
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.

3 participants