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

Support factory pattern for deployment #607

Closed
maguroid opened this issue Oct 29, 2023 · 4 comments · Fixed by #621
Closed

Support factory pattern for deployment #607

maguroid opened this issue Oct 29, 2023 · 4 comments · Fixed by #621
Assignees
Labels
status:ready This issue is ready to be worked on type:bug Something isn't working

Comments

@maguroid
Copy link

maguroid commented Oct 29, 2023

While trying out, I realized that the deployed contract addresses are not stored when using a factory contract for deployment. Additionally, the visualization failed with the following error:

TypeError: Cannot read properties of undefined (reading 'id')
    at file:///Users/maguroid/ghq/github.com/maguroid/hardhat-ignition-demo/cache_hardhat/visualization/index.html:326:356
    at Array.map (<anonymous>)
    at lEn (file:///Users/maguroid/ghq/github.com/maguroid/hardhat-ignition-demo/cache_hardhat/visualization/index.html:326:208)
    at bPe (file:///Users/maguroid/ghq/github.com/maguroid/hardhat-ignition-demo/cache_hardhat/visualization/index.html:70:20007)
    at N7t (file:///Users/maguroid/ghq/github.com/maguroid/hardhat-ignition-demo/cache_hardhat/visualization/index.html:72:44535)
    at M7t (file:///Users/maguroid/ghq/github.com/maguroid/hardhat-ignition-demo/cache_hardhat/visualization/index.html:72:40195)
    at Uhn (file:///Users/maguroid/ghq/github.com/maguroid/hardhat-ignition-demo/cache_hardhat/visualization/index.html:72:40118)
    at gge (file:///Users/maguroid/ghq/github.com/maguroid/hardhat-ignition-demo/cache_hardhat/visualization/index.html:72:39966)
    at iNe (file:///Users/maguroid/ghq/github.com/maguroid/hardhat-ignition-demo/cache_hardhat/visualization/index.html:72:36270)
    at A7t (file:///Users/maguroid/ghq/github.com/maguroid/hardhat-ignition-demo/cache_hardhat/visualization/index.html:72:35207)
    at le (file:///Users/maguroid/ghq/github.com/maguroid/hardhat-ignition-demo/cache_hardhat/visualization/index.html:57:1666)
    at MessagePort.pe (file:///Users/maguroid/ghq/github.com/maguroid/hardhat-ignition-demo/cache_hardhat/visualization/index.html:57:2056)

Here is my module for reproduction:
https://github.com/maguroid/hardhat-ignition-demo/blob/factory-pattern/ignition/modules/Vault.ts

I want to express my gratitude to the entire team for their hard work and support throughout this project. Your dedication has been invaluable.

@kanej
Copy link
Member

kanej commented Oct 30, 2023

@maguroid thanks for the kind words ... and the repo to help us reproduce the issues.

On the visualization error. I was able to get a minimally reproducible example where the module id of a submodule matches the module id of the root module. In this case both the vault module and the vault factory module have the same id. If I rename the vault factory module I get a visualization.

This is missing validation, you should have got a clear and comprehensible error on running visualize. I have opened a separate issue for this bug here: #608

Can I confirm @maguroid that that the deployed contract addresses are not stored when using a factory means you where expecting that the contractAt that wrap the readEventArguments from the factory deploys would be recorded into deployed_addresses.json?

So something like:

  const univ3Usdc = m.contractAt('Vault', m.readEventArgument(create1, 'VaultCreated', 'vault', { id: 'read1' }), { id: 'create1' })
  const mockUsdc = m.contractAt('Vault', m.readEventArgument(create2, 'VaultCreated', 'vault', { id: 'read2' }), { id: 'create2' })

  return {
    vaultFactory,
    univ3Usdc,
    mockUsdc,
  }

Shows the contracts deployed through the factory in the deploy command:

image

But the returned contractAt are missing (i.e. Vault#create1 and Vault#create2) from the deployed_addresses.json

@maguroid
Copy link
Author

@kanej
Thank you for the prompt response! I made a mistake by not specifying the ID, but I have now confirmed that the results you provided are consistent with my findings.

But the returned contractAt are missing (i.e. Vault#create1 and Vault#create2) from the deployed_addresses.json

Yes. It would be greatly appreciated if this issue could be resolved...

@kanej
Copy link
Member

kanej commented Oct 30, 2023

I think it makes sense to include contractAt addresses in deployed addresses; but let me check with the rest of the team and see if we can think through unintended consequences.

@kanej kanej added type:bug Something isn't working status:ready This issue is ready to be worked on and removed status:triaging labels Oct 31, 2023
@kanej kanej added this to the v0.12.0 Release milestone Oct 31, 2023
@kanej
Copy link
Member

kanej commented Oct 31, 2023

This is a bug, we suspect that our check for recording the deployed address should include the contractAt init message:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:ready This issue is ready to be worked on type:bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants