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: tool interfaces aka fasad #10

Merged
merged 25 commits into from
Jan 8, 2024
Merged

Conversation

BeroBurny
Copy link
Collaborator

This is a draft to create conversations based on some things
Resolves #4

@BeroBurny BeroBurny added Status: In Progress Added to issues to signal that its actively being worked on. Status: Needs Clarification Added to issues that are not clearly understood, and require additional input. labels Dec 4, 2023
packages/plugin/src/index.ts Show resolved Hide resolved
packages/plugin/src/index.ts Show resolved Hide resolved
packages/plugin/package.json Show resolved Hide resolved
@BeroBurny BeroBurny marked this pull request as ready for review December 6, 2023 06:37
},
multichain: {
environment: Environment.TESTNET,
deploymentNetworks: ["sepolia", "optimisticGoerli"],
Copy link
Member

@mpetrunic mpetrunic Dec 6, 2023

Choose a reason for hiding this comment

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

maybe it would be better to have something like:

   networks: {
        sepolia: { ... },
        goerli: { ... },
        mumbai: { ... },
        mainnet: { ... },
        polygon: { ... },
        optimism: { ... },
   }, 
   multichain: {
   networkConfig: {
     goerli: {
        deploymentTargets: ["sepolia", "Mumbai"]
     },
     optimism: {
        deploymentTargets: ["mainnet", "polygon"]
     }
   }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there is already network config from hardhat do not see the point of doing a double thing...
https://hardhat.org/tutorial/deploying-to-a-live-network#deploying-to-remote-networks

Copy link
Member

Choose a reason for hiding this comment

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

typo, "networkConfig" should be inside "multichain"


public constructor(private readonly hre: HardhatRuntimeEnvironment) {
this.initializationPromise = new Promise<void>((resolve, reject) => {
void this.initSygma(resolve, reject);
Copy link
Member

Choose a reason for hiding this comment

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

why do you have to do that in constructor?

Can't you have async function getConfig which checks if there is cached config and if there is not, inits sygma ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

validation: validate (using sygma SDK) that all routes exist between those networks

in the task list is to validate with Sygma if routes exist. It could not be done while initializing the config, so moved into the constructor so can be done async over there. It is not the most elegant approach, but is kinda one of the possible approaches that will reduce wait time for validation.

Example scenario: You've created an ERC20 contract and want to deploy it across multiple chains using the configuration mentioned above.
```typescript
// Wait for deployment initialization
await hre.multichain.waitInitialization();
Copy link
Member

Choose a reason for hiding this comment

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

a bit hacky? Why do we need this since "deployMultichain" is already async?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

We discussed removing this before the break^^


public async deployMultichain(
nameOrBytecode: string,
args: string[],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
args: string[],
contructorArgs: string[],

and use type for args from Web3.js

Copy link
Member

Choose a reason for hiding this comment

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

even better, use web3.js types to ensure typesafety of constructor args

await this.initializationPromise;
}

public async deployMultichain(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public async deployMultichain(
public async deployMultichain<ABI: Abi = any>(

public async deployMultichain(
nameOrBytecode: string,
args: string[],
options?: Object
Copy link
Member

Choose a reason for hiding this comment

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

maybe define object. You will need to use abi generic.
Since constructor args needs to remain the same for contract to get same address, usually you would invoke, a separate init method in the same tx.

So they would need to pass initi method name (use web3js typings to allow only methods define in Abi) and arguments for that method (again, you can use web3.js typings to ensure type safety.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not go deep into types for it as probably be less productive thinking about all possible types that create them when will are implemented

@BeroBurny BeroBurny requested a review from mpetrunic January 8, 2024 13:34
@mpetrunic mpetrunic merged commit 6351819 into master Jan 8, 2024
2 checks passed
@mpetrunic mpetrunic deleted the beroburny/interfaces-aka-fasade branch January 8, 2024 13:58
@github-actions github-actions bot mentioned this pull request Jan 11, 2024
BeroBurny pushed a commit that referenced this pull request Feb 16, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>hardhat-plugin-multichain-deploy: 1.0.0</summary>

## 1.0.0 (2024-02-16)


### Features

* adapter address env
([#25](#25))
([628a99c](628a99c))
* deploy multichain bytecode
([#22](#22))
([2eff1ee](2eff1ee))
* encode constructor improve
([#24](#24))
([9c4b7b1](9c4b7b1))
* encode function and deployMultichain flow
([#14](#14))
([26229cf](26229cf))
* encodeInitData
([#30](#30))
([50758ca](50758ca))
* example
([#49](#49))
([637d0ae](637d0ae))
* initLocalEnvironment
([#47](#47))
([e728a10](e728a10))
* rework networks input
([#23](#23))
([3caf657](3caf657))
* tool interfaces aka fasad
([#10](#10))
([6351819](6351819))
* transaction details and deployment info
([#33](#33))
([2b3232d](2b3232d))


### Bug Fixes

* deployment network type and naming
([#26](#26))
([1060069](1060069))
* small logical mistakes
([#48](#48))
([6cfd609](6cfd609))
</details>

<details><summary>hardhat-plugin-multichain-deploy-contracts:
1.0.0</summary>

## 1.0.0 (2024-02-16)


### Features

* add contracts implementation
([#11](#11))
([114c05f](114c05f))
* example
([#49](#49))
([637d0ae](637d0ae))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: In Progress Added to issues to signal that its actively being worked on. Status: Needs Clarification Added to issues that are not clearly understood, and require additional input.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugin interface and validation
2 participants