-
Notifications
You must be signed in to change notification settings - Fork 10
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
Packages update #44
Packages update #44
Conversation
Update `ethers` package to version 6.10.0 to be able to use it in more recent projects. Update `hardhat` related packages to sync with their latest versions. Update `typescript` related packages to be able to build project with updated dependencies.
As there were a lot of breaking changes between versions of `ethers` and `hardhat` related packages let's update codebase to use new versions of these libraries. Some problems with types are still existing here and should be solved later.
src/time.ts
Outdated
* @return {number} Timestamp of the next block. | ||
*/ | ||
export async function increaseTime( | ||
provider: JsonRpcProvider, | ||
time: BigNumberish | ||
): Promise<BigNumber> { | ||
time: number |
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 now I'm thinking we can go back to bigint here if needed, need to check what evm_setNextBlockTimestamp
wanted to receive
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.
I am just guessing but I believe the intention was to provide a single interface one can import to manipulate time and have this interface include mineBlocks
and mineBlocksTo
. I do not have a strong opinion personally, I think we could leave those functions in the interface but I would probably rework the implementation to use Hardhat functions underneath wherever possible.
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.
Hardhat network helpers were not available at the time this library was implemented.
I agree with @pdyraga's proposal.
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.
src/upgrades.ts
Outdated
// @ts-ignore-next-line | ||
receipt: transactionReceipt, | ||
// @ts-ignore-next-line | ||
libraries: opts?.factoryOpts?.libraries, |
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.
Problem with receipt
is that it is not reusing any of the ethers
transaction receipt types but it is written from scratch. Not sure how we want to approach type conversions here.
Problem with libraries
is that it is basically the same type (shared structure) but again, version inside Deployment
is a bit different than the FactoryOptions
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.
libraries
fixed by adjusting the type 8eece55
receipt
- no idea how to solve it for now as these types are incompatible, removed the receipt
for now as this is an optional field on Deployment
type. Not sure if we can leave it like that, end user should be able to get receipt if needed anyway because there is transactionHash
saved in the deployment 7adb77f
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.
What we wanted to achieve was to output deployment artifacts matching the output of the hardhat-helpers
plugin. Unfortunately hardhat-helpers
are still based on ethers v5. I'm fine with the temporary removal of the receipt
property.
src/number.ts
Outdated
export function to1ePrecision(n: any, precision: number): BigNumber { | ||
const decimalMultiplier = BigNumber.from(10).pow(precision) | ||
return BigNumber.from(n).mul(decimalMultiplier) | ||
export function to1ePrecision(n: bigint, precision: number): bigint { |
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.
We could improve the to1e18
and to1ePrecision
functions and allow string | number | bigint
as n
input.
export function to1ePrecision(
n: string | number | bigint,
precision: number,
): bigint {
return BigInt(n) * 10n ** BigInt(precision)
}
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.
647ceed - used BigNumberish
type as this is doing the same thing
src/signers.ts
Outdated
} | ||
|
||
export interface HardhatSignersHelpers { | ||
getNamedSigners(): Promise<NamedSigners> | ||
getUnnamedSigners(): Promise<SignerWithAddress[]> | ||
getUnnamedSigners(): Promise<Signer[]> |
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.
In ethers v6 SignerWithAddress
was migrated to HardhatEthersSigner
.
Could we use this type instead of the Signer
interface?
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.
Oh you're right, it's even aliased as SignerWithAddress
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.
As `SignerWithAddress` is still available - let's use it instead of ethers' `Signer` type
As `receipt` type of `deployment` object is not compatible with `transactionReceipt` let's not include it in the deployment for now as user will be able to get the receipt by `transactionHash` included in the `deployment` object anyway.
src/time.ts
Outdated
* @return {number} Timestamp of the next block. | ||
*/ | ||
export async function increaseTime( | ||
provider: JsonRpcProvider, | ||
time: BigNumberish | ||
): Promise<BigNumber> { | ||
time: number |
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 am just guessing but I believe the intention was to provide a single interface one can import to manipulate time and have this interface include mineBlocks
and mineBlocksTo
. I do not have a strong opinion personally, I think we could leave those functions in the interface but I would probably rework the implementation to use Hardhat functions underneath wherever possible.
Helpers from `time` utils are rewritten to use hardhat's implementations. These utils should behave the same as before.
Release 0.7.0 Version update to 0.7.0 after updating packages to latest versions in #44
Why
Update of core packages like
ethers
andhardhat
to makehardhat-helpers
package compatible with projects using new versions of these packages.What
Packages updates:
.nvmrc
with specified node versionv20.10.0
as node older than LTS won't support new packagestypescript
to version>5
(and related packages) to be able to compile these new packageseslint
and related packages to correctly lint codebasehardhat
packages to newest versions:hardhat-etherscan
withhardhat-verify
ethers
package to version>6
Codebase updates:
BigNumbers
- as ethers v6 is not longer usingBigNumbers
they are replaced with nativebigints
calculationsethers
, sometimes it means that the function or type is slightly changed, moved out ofutils
or renamedSignerWithAddress
were removed completely and they were replaced with similar typesnull
orundefined
which broke our code - optional chaining with default values has been used to fix these placesRemaining problems:
upgrades
file - and we should make sure their test coverage was enough to be sure they are working correctly