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

feat: make typegen factories an object instance #2359

Merged
merged 5 commits into from
May 21, 2024

Conversation

nedsalk
Copy link
Contributor

@nedsalk nedsalk commented May 21, 2024

typegen factories currently are classes with static methods which is a problem when wanting to pass the whole factory as an argument because e.g. passing a whole TestContract__factory as an argument and using it in the manner of factoryArgument.deployContract would yield an undefined.deployContract error because TestContract__factory is not an object instance. This PR is necessary to be able to pass the outputs of typegen to launchTestNode in #1356. It doesn't break anything for end consumers.

@nedsalk nedsalk added the feat Issue is a feature label May 21, 2024
@nedsalk nedsalk self-assigned this May 21, 2024
@nedsalk nedsalk changed the title Ns/feat/make typegen output passable feat: make typegen factories an object instance May 21, 2024
@nedsalk nedsalk enabled auto-merge (squash) May 21, 2024 11:30
@Torres-ssf
Copy link
Contributor

@nedsalk Does the described issue with Typegen factories indicate a design flaw, or is this change primarily to fit the needs of launchTestNode?

Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
79.73%(+0%) 70.64%(-0.03%) 76.81%(-0.01%) 79.81%(+0%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/account/src/account.ts 84.14%
(+1.36%)
67.16%
(+0.5%)
80.64%
(+4.64%)
83.73%
(+1.38%)
🔴 packages/account/src/providers/transaction-request/transaction-request.ts 87.32%
(+1.41%)
77.63%
(+2.63%)
86%
(+0%)
87.67%
(+1.37%)

Copy link
Contributor

@petertonysmith94 petertonysmith94 left a comment

Choose a reason for hiding this comment

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

I like this approach. Would it be worth having interfaces for these factories?

@nedsalk
Copy link
Contributor Author

nedsalk commented May 21, 2024

Does the described issue with Typegen factories indicate a design flaw, or is this change primarily to fit the needs of launchTestNode?

@Torres-ssf This change is primarily to fit the needs of launchTestNode. The design itself is okay as it follows ContractFactory. There is possibility for improvement here so that we don't need e.g. the contract as unknown as ContractAbi casts, but that's more related to the internal design of how we integrate typegen outputs with the rest of our codebase.

I like this approach. Would it be worth having interfaces for these factories?

@petertonysmith94 No because these factories themselves follow the interface of the ContractFactory class. Ideally we'd be returning a ContractFactory<TTypegenAbi> instance and we wouldn't have the need for this *__factory object, but this is related to what I told @Torres-ssf above - an internal design restructuring.

@nedsalk nedsalk merged commit 6d1db46 into master May 21, 2024
19 checks passed
@nedsalk nedsalk deleted the ns/feat/make-typegen-output-passable branch May 21, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Issue is a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants