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

Challenge 5 migration to extension #250

Open
wants to merge 6 commits into
base: challenge-5-state-channels--extension
Choose a base branch
from

Conversation

damianmarti
Copy link
Member

Challenge 5 migrated to an extension following #234

Install from create-eth repo with:

yarn cli -e scaffold-eth/se-2-challenges:ch5-extension-init

closes #246

@rin-st
Copy link
Member

rin-st commented Dec 16, 2024

Houston, we have a problem

https://github.com/scaffold-eth/se-2-challenges/blob/challenge-5-state-channels/packages/nextjs/services/web3/wagmiConnectors.tsx#L1

In this challenge we don't use burner-wallet package, I used sessionStorage version instead of localStorage used in burner-wallet. I did it because we needed to get a new user in every browser tab.

So, options we have

  • (most easy) ask to open Rubes in a new private browser windows
  • update burner-wallet and add possibility to use sessionStorage
  • try to use the same sessionStorage version
  • ... something different


## Checkpoint 0: 📦 Environment 📚

> in the same terminal, start your local network (a blockchain emulator in your computer):
Copy link
Member

@rin-st rin-st Dec 16, 2024

Choose a reason for hiding this comment

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

remove "In the same terminal" part, pls check if you had that in previous extensions too

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

Yes, it is the same as in some of the previous extensions I will fix it there too!

@damianmarti
Copy link
Member Author

Houston, we have a problem

https://github.com/scaffold-eth/se-2-challenges/blob/challenge-5-state-channels/packages/nextjs/services/web3/wagmiConnectors.tsx#L1

In this challenge we don't use burner-wallet package, I used sessionStorage version instead of localStorage used in burner-wallet. I did it because we needed to get a new user in every browser tab.

So, options we have

  • (most easy) ask to open Rubes in a new private browser windows
  • update burner-wallet and add possibility to use sessionStorage
  • try to use the same sessionStorage version
  • ... something different

oh!

I think that we can go with the easier solution, asking to open the other user in a private browser window...

@rin-st
Copy link
Member

rin-st commented Dec 16, 2024

I think that we can go with the easier solution, asking to open the other user in a private browser window...

Yes, let's try to do it that way. Update Readme with that info please. Keep in mind we need a new private browser window for every rube, different tabs of one window also doesn't work

@damianmarti
Copy link
Member Author

I think that we can go with the easier solution, asking to open the other user in a private browser window...

Yes, let's try to do it that way. Update Readme with that info please. Keep in mind we need a new private browser window for every rube, different tabs of one window also doesn't work

Checking the README I read:

⚠️ Note: previous challenges created new addresses by opening an incognito window or a different browser. This will not work for this challenge, because the off-chain application uses a very simple communication pipe that doesn't work between different browsers or private windows.

What is the limitation to making it work on different browsers or private windows?

@rin-st
Copy link
Member

rin-st commented Dec 16, 2024

Oh, it uses https://developer.mozilla.org/en-US/docs/Web/API/Broadcast_Channel_API (grep BroadcastChannel in the app).

As I remember it worked only with tabs before, but now I see in the docs:

"The Broadcast Channel API allows basic communication between browsing contexts (that is, windows, tabs, frames, or iframes) and workers on the same origin."

So probably windows also works. Will try it tomorrow

@rin-st
Copy link
Member

rin-st commented Dec 16, 2024

Tried fast check, doesn't work (. Will play with it tomorrow

Also, this challenge uses ethers, humanize-duration and @types/humanize-duration deps. Uncomment checkpoint 4 and you'll see

@damianmarti
Copy link
Member Author

Tried fast check, doesn't work (. Will play with it tomorrow

I tried the broadcast channel with the incognito window and it's not working :-(

We will need to go with one of the other options:

  1. update burner-wallet and add the possibility to use sessionStorage
  2. try to use the same sessionStorage version
  3. ... something different

Maybe we can try option 1.

Another option is to remove the broadcast channel and use a backend for the messages.

But adding the sessionStorage to the burner wallet may be something useful for other projects too.

@rin-st what do you think?

@rin-st
Copy link
Member

rin-st commented Dec 17, 2024

Maybe we can try option 1.

Yes, I'll try to update it later today

@damianmarti
Copy link
Member Author

Maybe we can try option 1.

Yes, I'll try to update it later today

Great! Thanks!

I will try to remove the ethers dependency, it seems like the missing function from viem is already implemented https://viem.sh/docs/utilities/parseSignature

@damianmarti
Copy link
Member Author

Looking at the burner wallet code from the previous challenge 5

Maybe we can try option 1.

Yes, I'll try to update it later today

Great! Thanks!

I will try to remove the ethers dependency, it seems like the missing function from viem is already implemented https://viem.sh/docs/utilities/parseSignature

Removed viem dependency using hexToSignature (we should update it to use parseSignature when updating viem).

Added humanize-duration dependency too and fixed some minor code comments.

@technophile-04
Copy link
Collaborator

Houston, we have a problem

https://github.com/scaffold-eth/se-2-challenges/blob/challenge-5-state-channels/packages/nextjs/services/web3/wagmiConnectors.tsx#L1

In this challenge we don't use burner-wallet package, I used sessionStorage version instead of localStorage used in burner-wallet. I did it because we needed to get a new user in every browser tab.

I think its a nice feature to have and we can add it, since changes are not that much. Thanks @rin-st for the burner-connector PR 🙌

@rin-st
Copy link
Member

rin-st commented Dec 20, 2024

Removed viem dependency using hexToSignature (we should update it to use parseSignature when updating viem).

Great! But why not to use parseSignature right away? Our viem version allows it

We updated burner-connector to next version so it can work with sessionStorage. I tried it with this challenge and now it works as expected. But to make it installed by default we need to wait for scaffold-eth/scaffold-eth-2#1021 to be merged. Also, after that merge, we need to add this to this challenge READMESs to make it work


add this line to wagmiConnectors.ts to use sessionStroage version and test:

rainbowkitBurnerWallet.useSessionStorage = true;

@damianmarti
Copy link
Member Author

Removed viem dependency using hexToSignature (we should update it to use parseSignature when updating viem).

Great! But why not to use parseSignature right away? Our viem version allows it

I think the viem version we are using does not have parseSignature yet. I tried it before changing to hexToSignature and the method does not exist.

We updated burner-connector to next version so it can work with sessionStorage. I tried it with this challenge and now it works as expected. But to make it installed by default we need to wait for scaffold-eth/scaffold-eth-2#1021 to be merged.

Great!! Thanks!!

Also, after that merge, we need to add this to this challenge READMESs to make it work

add this line to wagmiConnectors.ts to use sessionStroage version and test:

rainbowkitBurnerWallet.useSessionStorage = true;

I will add this line to the READMEs!

@damianmarti
Copy link
Member Author

@rin-st Added the rainbowkitBurnerWallet config to READMEs!

The PR you mentioned was merged, but now we have to wait to get this update on create-eth, right?

@rin-st
Copy link
Member

rin-st commented Dec 20, 2024

I think the viem version we are using does not have parseSignature yet. I tried it before changing to hexToSignature and the method does not exist.

It works for me 🤷 . Added it bb5cdf9, please try on your side

The PR you mentioned was merged, but now we have to wait to get this update on create-eth, right?

Yes, sorry :)

@damianmarti
Copy link
Member Author

I think the viem version we are using does not have parseSignature yet. I tried it before changing to hexToSignature and the method does not exist.

It works for me 🤷 . Added it bb5cdf9, please try on your side

Working fine! Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants