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: prop access and added test #231

Merged
merged 2 commits into from
May 31, 2024

Conversation

Keyrxng
Copy link
Contributor

@Keyrxng Keyrxng commented May 29, 2024

Resolves #227

  • correct prop access typo
  • add test for invalidate functionality

@Keyrxng Keyrxng requested a review from pavlovcik as a code owner May 29, 2024 01:45
@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented May 29, 2024

Copy link
Contributor

github-actions bot commented May 29, 2024

@Keyrxng
Copy link
Contributor Author

Keyrxng commented May 29, 2024

CI failed because of nonce too low again

image

@rndquu
Copy link
Member

rndquu commented May 29, 2024

CI failed because of nonce too low again

image

Could you merge the latest development branch and try again?

@Keyrxng
Copy link
Contributor Author

Keyrxng commented May 29, 2024

@rndquu all passing, the new setup scripts seem like they are doing the trick. Ofc it could still potentially fail but it's far less likely

@0x4007
Copy link
Member

0x4007 commented May 29, 2024

Preview Deployment
25555f1d2b7267a86821633da74930418f611bfa
f6b6f8581ab56185931c6e26ee56e62b146616cf

@gentlementlegen perhaps we should remove this feature because it doesn't work where it's needed. I was going to test the pull now but the link is invalid. Or if you can fix this that would be ideal.

Alternatively we could create a "dev mode" menu on preview deployments to inject a permit. Perhaps we could generate it from the current active wallet so that we could also test invalidation etc. just a thought but also I'm a bit concerned that it's over engineered for the problem we are trying to solve (QA)

Reopened #195

@gentlementlegen
Copy link
Member

@0x4007 Sadly back to the problem with the env missing on pull_request unlike pull_request_target. I'll see if we can use the same trick we do for Knip testing, where we trigger another workflow that will get the environment loaded. Otherwise we will have to add a dev mode indeed.

@rndquu rndquu self-requested a review May 31, 2024 07:16
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

@rndquu rndquu requested a review from gentlementlegen May 31, 2024 07:48
Copy link
Member

@gentlementlegen gentlementlegen left a comment

Choose a reason for hiding this comment

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

Ran tests locally, all successful. My only remark would be that maybe http://localhost:8545 deserves to be stored in a variable, it seems to be used in plenty of places.

@rndquu rndquu merged commit eccb5f0 into ubiquity:development May 31, 2024
4 checks passed
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.

Permit Invalidation Does Not Work
4 participants