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

Simplify converting EVM addresses to oasis1 #1365

Merged
merged 6 commits into from
Apr 5, 2024
Merged

Conversation

lukaw3d
Copy link
Member

@lukaw3d lukaw3d commented Apr 5, 2024

No description provided.

Copy link

github-actions bot commented Apr 5, 2024

Deployed to Cloudflare Pages

Latest commit: 3d80bafb9877dd0bc257012d97eac0cc27f75ce7
Status:✅ Deploy successful!
Preview URL: https://c6bbb62b.oasis-explorer.pages.dev

src/app/components/Search/search-utils.ts Show resolved Hide resolved
@@ -104,7 +103,6 @@ export const routes: RouteObject[] = [
{
path: '/search', // Global search
element: withDefaultTheme(<SearchResultsPage />),
loader: searchParamLoader,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we removing the searchParamLoader here? I would rather see that app fails in a loader if invalid URL, instead of in component.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think searchParamLoader was validating the URL. We just needed to parse the query in URL, but async code forced us to use a loader (or useEffects)

Copy link
Collaborator

Choose a reason for hiding this comment

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

const network = useNetworkParam()
if (!!network && !RouteUtils.getEnabledNetworks().includes(network)) {
throw new AppError(AppErrors.InvalidUrl)
}

To me it seems like it throws, if the scope isn't specified.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should have linked to master, either way, this code was previously in searchParamLoader

Copy link
Member Author

Choose a reason for hiding this comment

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

Checked more thoroughly

Before:

  • /:_network/:_layer/search:
    • /:_network/:_layer/: assertEnabledScope validated network and scope
    • /:_network/:_layer/search: searchParamLoader validated network again
  • /search: searchParamLoader skipped the validation because !!network is false

After:

  • /:_network/:_layer/search:
    • /:_network/:_layer/: assertEnabledScope validates network and scope
  • /search: nothing to validate

Copy link
Member Author

Choose a reason for hiding this comment

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

(found #1366 while checking)

src/app/components/Search/search-utils.ts Show resolved Hide resolved
Copy link
Collaborator

@lubej lubej left a comment

Choose a reason for hiding this comment

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

LGTM, just fix the linting issues

@lukaw3d lukaw3d enabled auto-merge April 5, 2024 11:35
@lukaw3d lukaw3d merged commit 8642178 into master Apr 5, 2024
8 checks passed
@lukaw3d lukaw3d deleted the lw/simplify-convert branch April 5, 2024 11:37
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.

None yet

2 participants