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

1568 nonce page refresh #1969

Merged
merged 3 commits into from
Jun 5, 2024
Merged

1568 nonce page refresh #1969

merged 3 commits into from
Jun 5, 2024

Conversation

Da-Colon
Copy link
Contributor

@Da-Colon Da-Colon commented Jun 3, 2024

Fixes #1568

Not sure if we should also cache and/or exclude routes. but move the implementation done in the New Proposals page to the DAOController and set it up to make the request whenever the url changes.

The alternative to this, would be to refactor this hook back to the isMounted ref setup and add the hook to the particular pages/models/components that may create a proposal. But then we have to add it to any new pages/components that may need this features. Thoughts?

@Da-Colon Da-Colon self-assigned this Jun 3, 2024
Copy link

netlify bot commented Jun 3, 2024

Deploy Preview for decent-interface-dev ready!

Name Link
🔨 Latest commit e872605
🔍 Latest deploy log https://app.netlify.com/sites/decent-interface-dev/deploys/665d0fdac8dd860008877c5d
😎 Deploy Preview https://deploy-preview-1969.app.dev.decentdao.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Da-Colon Da-Colon requested review from adamgall, DarksightKellar, mudrila and a team June 3, 2024 14:21
Copy link

@tomstuart123 tomstuart123 left a comment

Choose a reason for hiding this comment

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

will leave to devs to answer your questions.

Send assets modal seems to be performing as desired when it comes to nonce proposals in pending though so approved from my side

@adamgall
Copy link
Member

adamgall commented Jun 4, 2024

This is a good general solution to our problem, for sure.

A part of me is on the fence about whether this top-down approach is the right way, or if the approach you outlined as an alternative would be better.

"General solution that might not be very efficient" vs "precision solution that won't waste network calls".

Copy link
Member

@adamgall adamgall left a comment

Choose a reason for hiding this comment

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

Adding approval because this does seem to work well, but would like to hear your thoughts @Da-Colon on if you think the alternative solution is "better". There are tradeoffs in both directions of course.

@Da-Colon
Copy link
Contributor Author

Da-Colon commented Jun 4, 2024

Adding approval because this does seem to work well, but would like to hear your thoughts @Da-Colon on if you think the alternative solution is "better". There are tradeoffs in both directions of course.

Better as far as maybe more fine tuned say, when you open a model, or when you go to the final step to create a proposal, it makes the request so its the most up to date as possible.

IMO it doesn't need to be fine tuned right now. Until we get a user saying its off or until we tackle updates to global state

}

if (prevPathname.current !== location.pathname) {
(async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

For me self-calling function makes it little bit harder to read - I'd suggest changing this to named function and then call it.

So something like async function setSafeInfo () { blah } and then setSafeInfo()

@Da-Colon Da-Colon merged commit 72055ce into develop Jun 5, 2024
7 checks passed
@Da-Colon Da-Colon deleted the issue/1568-nonce-refresh branch June 5, 2024 15:14
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.

Make sure correct nonce is being used on Send Assets proposal creation modal
5 participants