-
Notifications
You must be signed in to change notification settings - Fork 193
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
Challenge 4 migration to extension #245
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.
Thanks @rin-st for the review and great catch! I fixed it by adding a package.json to hardhat with @openzeppelin/contracts. The new @openzeppelin/contracts v5 uses custom errors and our getParsedError is not able to show a nice message in this case. We should add an issue to fix it. |
Lgtm! Created an issue to fix that error, see above. Thank you @damianmarti ! |
extension/README.md.args.mjs
Outdated
|
||
### ⚔️ Side Quests | ||
|
||
- [ ] In \`packages\nextjs\app\events\page.tsx\` implement an event and emit for the \`approve()\` function to make it clear when it has been executed. |
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.
`packages\nextjs\app\events\page.tsx` this is getting rendered badly in my instance readme:
In `packages
extjsappeventspage.tsx`
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 think there was a backslash escaping problem here, was still seeing it wrong.
As a quick fix just changed the backslash for regular slash in a9a4a12 since we're using regular slashes in other folders.
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.
Nice job Damu!! 🙌 The challenge is working great to me.
Just added some comments with nitpick format errors in the Readme from my instance.
Also just to comment it, I received this message when I tried to yarn vercel
. Never saw it before, I needed to do yarn vercel --yes
:
$ yarn vercel
Vercel CLI 39.1.3
> NOTE: The Vercel CLI now collects telemetry regarding usage of the CLI.
> This information is used to shape the CLI roadmap and prioritize features.
> You can learn more, including how to opt-out if you'd not like to participate in this program, by visiting the following URL:
> https://vercel.com/docs/cli/about-telemetry
Error: Command `vercel deploy` requires confirmation. Use option "--yes" to confirm.
Could you check if it asking it for new deployments? Probably you need to create new instance, or is it asked just one time? Also, try it with env variables like in this PR |
Just created a fresh instance of another extension and seems like today I'm getting another error, without the telemetry promt: Tried with PR but still get the message, I needed to add the |
from the docs :
So as I understand it should be a question before asking for Could you please try one more thing for the last project:
and then
Is it still asking for |
Thanks @Pabl0cks for the review!! I used a clean README and pushed the changes. There was an issue with the previous one due to auto-formatting from my IDE. Please, let me know if the formatting is ok now. |
I'm using Vercel CLI 37.4.2 and it's working fine. Maybe some wrong project configuration. Can you please try to do it from a fresh project? |
Realized it was only happening to my Git Bash (on desktop). The rest of terminals were working well, even Git Bash on laptop. |
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.
Looking good to merge! TY @damianmarti
Challenge 4 migrated to an extension following #234
Install from create-eth repo with:
closes #241