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

From Call Data with proxy.proxy should have option to NOT override. #613

Closed
peterwht opened this issue Dec 27, 2024 · 7 comments · Fixed by #616
Closed

From Call Data with proxy.proxy should have option to NOT override. #613

peterwht opened this issue Dec 27, 2024 · 7 comments · Fixed by #616
Assignees
Labels
Priority: 🟠 P1 Added to issues and PRs relating to a high severity bugs. Type: 🏗️ Feature Added to issues and PRs to identify that the change is a new feature.

Comments

@peterwht
Copy link

peterwht commented Dec 27, 2024

The setup is Multisig -> Pure Proxy 1 -> Pure Proxy 2 .

Multix doesn't support creating calls for Pure Proxy 2 (would be a great feature, but it diverges from the current issue :) ). This requires manually creating the proxy.proxy call on p.js. The call data is then copied and provided to Multix for Pure Proxy 1. From Call Data is selected, with the encoded call data including proxy.proxy. Multix gives a warning saying "Multix will override the proxy.proxy call with the proxy you have selected".

Image

This makes it so the entire transaction now has to be created via p.js because we need:

multisig 
    proxy.proxy
        proxy.proxy
             <some_call>
@Tbaut
Copy link
Collaborator

Tbaut commented Dec 28, 2024

Hi, this is originally a feature, because many users have a tendency to copy/paste a call with proxy, although Multix takes care of this. It feels a little like a feature creep, but I think I'll let you remove this override with a click.

@Tbaut Tbaut added Priority: 🟠 P1 Added to issues and PRs relating to a high severity bugs. Type: 🏗️ Feature Added to issues and PRs to identify that the change is a new feature. labels Dec 28, 2024
@peterwht
Copy link
Author

Hi, this is originally a feature, because many users have a tendency to copy/paste a call with proxy, although Multix takes care of this. It feels a little like a feature creep, but I think I'll let you remove this override with a click.

gm! Thank you. It definitely makes sense to prevent the foot gun. Maybe there can be a check to see if the call data is using the proxy in multix (and hence should be overwritten), or if the proxy is a separate proxy (and should not be overwritten)

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 3, 2025

oh, this is a very good idea. I'll see how feasible it is, I like the elegance of the solution :)

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 6, 2025

I've made a PR and you'll be able to play with the preview URL soon.
The only thing I'm worried about, is if users make a proxy call with the wrong proxy, and expect Multix to filter it. It won't be filtered any more, but on the other side, it'll be clearly displayed.. I hope I'm not shooting anyone in the foot here.

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 6, 2025

here's the preview @peterwht please let me know if this is what you've been expecting: https://51fb3293.multix.pages.dev/

@Tbaut Tbaut self-assigned this Jan 6, 2025
@peterwht
Copy link
Author

peterwht commented Jan 8, 2025

here's the preview @peterwht please let me know if this is what you've been expecting: https://51fb3293.multix.pages.dev/

Thanks for getting this added @Tbaut. I checked the preview URL, and this is exactly what we needed.
When pasting call data that contains a different proxy, it shows the Proxy.proxy call using another proxy (double proxy).
Image

Can confirm it shows up in the wallet preview as well:
Image

And then when using our proxy registered in Multix, it will properly overwrite the call and just show the underlying call (in this case, balances.transferKeepAlive).

@Tbaut Tbaut closed this as completed in #616 Jan 8, 2025
@Tbaut
Copy link
Collaborator

Tbaut commented Jan 8, 2025

Amazing, thanks for the reply. It's merged and will be live in a couple minutes ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 🟠 P1 Added to issues and PRs relating to a high severity bugs. Type: 🏗️ Feature Added to issues and PRs to identify that the change is a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants