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

fix(twap): cache fb handler verification for 10min #5200

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

shoom3301
Copy link
Collaborator

@shoom3301 shoom3301 commented Dec 13, 2024

Summary

Related to #2949

Since Safe fallback handler might be changed any time, we should not cache the fb verification result for ever.
In this PR I added cache life time - 10 minutes.

To Test

  1. Open a Safe with no CowSwapFallbackHandler
  2. Create a TWAP along with changing FB
  • within the next 10 minutes there should not be "FALLBACK HANDLER CHECKED" in console logs
  • after 10 mins (might need a page reload) there should be "FALLBACK HANDLER CHECKED" in console logs

@shoom3301 shoom3301 added the RELEASE Included in the release that is being closed label Dec 13, 2024
@shoom3301 shoom3301 self-assigned this Dec 13, 2024
Copy link

vercel bot commented Dec 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
cosmos ✅ Ready (Inspect) Visit Preview Dec 13, 2024 0:07am
cowfi ✅ Ready (Inspect) Visit Preview Dec 13, 2024 0:07am
explorer-dev 🛑 Canceled (Inspect) Dec 13, 2024 0:07am
sdk-tools 🛑 Canceled (Inspect) Dec 13, 2024 0:07am
swap-dev ✅ Ready (Inspect) Visit Preview Dec 13, 2024 0:07am
widget-configurator ✅ Ready (Inspect) Visit Preview Dec 13, 2024 0:07am

@@ -25,12 +25,14 @@ export async function verifyExtensibleFallback(
try {
const domainVerifier = await signatureVerifierContract.callStatic.domainVerifiers(safeAddress, domainSeparator)

console.log('FALLBACK HANDLER CHECKED, domainVerifier: ', domainVerifier)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO: remove after testing

Copy link
Collaborator

Choose a reason for hiding this comment

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

Forgot to remove?

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

It works for me, but why not using joitai?

@shoom3301
Copy link
Collaborator Author

shoom3301 commented Dec 13, 2024

but why not using joitai?

@anxolin this is a good question.
Because jotai atom by default returns an initial values always, and only after some time it fills the atom state with a value from localStorage.
So, with using jotai we would need to distinguish if the value is initial because of loading or because there is no value in localStorage.
In conclusion, using atomWithStorage() in cases like this one is a pain.

@anxolin
Copy link
Contributor

anxolin commented Dec 13, 2024

fine, not big deal, just saying the initial value could be:

  • undefined / null
  • OR a very old date (so it updates the 1st time)

@shoom3301
Copy link
Collaborator Author

@anxolin waaaait, I actually forgot about getJotaiMergerStorage() helper that @alfetopito added in the past

@shoom3301 shoom3301 merged commit c09f073 into main Dec 13, 2024
12 of 13 checks passed
@shoom3301 shoom3301 deleted the hotfix/twap-handler-check branch December 13, 2024 13:13
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
RELEASE Included in the release that is being closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants