-
Notifications
You must be signed in to change notification settings - Fork 14
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
Migrate utils/
JS files to TS
#183
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #183 +/- ##
========================================
Coverage 84.50% 84.50%
========================================
Files 35 35
Lines 1084 1084
Branches 221 221
========================================
Hits 916 916
Misses 168 168 ☔ View full report in Codecov by Sentry. |
message: Record<string, any>, | ||
domain: TypedDataDomain, | ||
): string { | ||
const typedDataDomain: TypedDataDomain = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const typedDataDomain: TypedDataDomain = { | |
const typedDataDomain = { |
This is a self-inferred type. It's useless to specify it. But we can keep it if you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal is to enforce respecting properties of the type when constructing the object.
@@ -77,9 +87,9 @@ const TYPES = { | |||
], | |||
}; | |||
|
|||
function buildTypes(primaryType) { | |||
function buildTypes(primaryType: string): Types { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function buildTypes(primaryType: string): Types { | |
function buildTypes(primaryType: string) { |
idem
const OPERATION = 'Operation'; | ||
const types = { | ||
const types: Types = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const types: Types = { | |
const types = { |
idem
interface Types { | ||
[key: string]: Array<TypedDataField>; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interface Types { | |
[key: string]: Array<TypedDataField>; | |
} |
This is a self-inferred type. It's useless to specify it. But we can keep it if you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually here is needed, and I prefer keeping it
await (await txPromise).wait(); | ||
} | ||
|
||
export class EthersDeployer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to understand the need for having a custom Deployer class. Why not use typechain Factories to deploy something ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this file is just here to handle if the factory smart contract is deployed and if so get the address of the factory or deploy it correctly
We could change the architecture of EthersDeployer / FactoryDeployerHelper but it's not the purpose of this PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a quick note, here the "Factory" is not the same thing as Typechain factories.
Here we use a generic factory deployer contract to deploy contracts at the same address cross-chains.
if (wallet.address) { | ||
signerPromise = hre.ethers.getSigner(wallet.address); | ||
} else { | ||
reject(new Error('Wallet address is undefined')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good for me, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding this file 🙏.
We can reference it in docs/README.md
:
# ENS
Registered ENS names on Bellecour or Mainnet are documented in [./ENS-Addresses.md](./ENS-Addresses.md)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we can add addresses for each name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
docs/ENS-Addresses.md
Outdated
- `core.v5.iexec.eth` - Core protocol proxy (ERC1538Proxy) | ||
- `apps.v5.iexec.eth` - App registry contract | ||
- `datasets.v5.iexec.eth` - Dataset registry contract | ||
- `workerpools.v5.iexec.eth` - Workerpool registry contract |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `workerpools.v5.iexec.eth` - Workerpool registry contract | |
- `workerpools.v5.iexec.eth` - Workerpool registry contract | |
To get more details, see [1_deploy-ens.ts script](deploy/1_deploy-ens.ts). |
FactoryDeployer.js
→FactoryDeployer.ts
constants.js
→constants.ts
odb-tools.js
→odb-tools.ts