-
Notifications
You must be signed in to change notification settings - Fork 433
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
fix: Add addOwner route and open tx flow #2399
Conversation
Branch preview✅ Deploy successful! |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
One potential issue I found is that if the user presses Next too fast, the core-sdk might not be loaded yet and it errors out. To fix this we would have to add the |
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.
Looks good. Please run yarn routes
to update the routing table.
Tested, it works. ✅ |
The cases described in the description are fine If the address of the owner is invalid it will prefill it still, but validate the error. Tested this for invalid addresses, addreses that are already owners, the safe itself, wrongly checksummed addresses, addresses with the wrong prefix (addresses with no prefix will adopt the prefix of the current network) If no owner address is in the URL it takes just to the settings of that safe. This is for both; if the address parameter is not in the URL or if the address parameter has no value |
Thanks for testing @francovenica!
I think this is also how it is on prod. For me it shows an error: https://add_owner--walletweb.review-wallet-web.5afe.dev/settings/setup?safe=nothing
It is also possible to get into this state when you disconnect your wallet after going into a tx-flow so I suggest to leave it as it is. |
Ah, ok, so these are existing issues. I'll report the wrong safe address not being rejected by the app in another ticket. We can pass this one |
What it solves
Resolves #2381
How this PR fixes it
/addOwner
that redirects to the/settings/setup
pageHow to test it
/addOwner?safe=<your safe address>&address=<the address you want to add as an owner>
/addOwner?safe=<your safe address>
/addOwner?safe=<your safe address>&address=<an invalid address>
Screenshots
Checklist