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: remove domain overwrite on simple redirects #977

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

crisog
Copy link
Contributor

@crisog crisog commented Oct 18, 2022

This is a complementary PR for #975.

The way that simple redirects were designed does not allow for a nice way to log the redirect domain, because it gets overwritten for the specific blockchain alias. This PR removes this domain overwrite to be able to log the redirect domain correctly.

@height
Copy link

height bot commented Oct 18, 2022

This pull request has been linked to 1 task:

💡Tip: Add "Close T-14959" to the pull request title or description, to a commit message, or in a comment to mark this task as "Done" when the pull request is merged.

@crisog
Copy link
Contributor Author

crisog commented Oct 18, 2022

Link T-14959

Copy link
Contributor

@adshmh adshmh left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. A few comments to address.

src/controllers/v1.controller.ts Show resolved Hide resolved
src/controllers/v1.controller.ts Show resolved Hide resolved
src/utils/relayer.ts Show resolved Hide resolved
@crisog crisog changed the title fix: domain overwrite simple redirect fix: remove domain overwrite on simple redirects Oct 19, 2022
@kutoft
Copy link

kutoft commented Oct 19, 2022

Link T-15008

Copy link
Contributor

@rem1niscence rem1niscence left a comment

Choose a reason for hiding this comment

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

LGTM

@kutoft
Copy link

kutoft commented Nov 1, 2022

Link T-15008

@crisog
Copy link
Contributor Author

crisog commented Nov 14, 2022

#984 ENV can be removed when this gets merged.

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.

4 participants