-
Notifications
You must be signed in to change notification settings - Fork 20
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
refactor: downgrade to ethers@v5 #70
Conversation
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.
Most of the changes generally look fine
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.
Most of the changes generally look fine
I agree if anything was wrong with the downgrade the tests would have broken, can't really QA it much beyond that unless going the extra mile and testing it against the various overloads or using it to generate a permit (I don't think I've ever actually generated a QA permit before in any repo other than pay.ubq.fi
and it uses it's own script, not this pkg. My attempts to QA conversation rewards all fail at the point of permit generation too so)
@0x4007 is this good to merge? |
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.
Let's test in production!
Resolves #60
I had to refactor a couple functions in
generate-erc20-permit.ts
due to issues with cognitive complexity, specifically I created three new functionsgetPrivateKey
getAdminWallet
andgetTokenDecimals
I also had to include a yarn packageManager version in
package.json
because it wasn't letting me commit otherwise. let me know if you want the yarn version changed, I used whatever version corepack set.