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

BCF-2901 Fix potential JAID collisions in multi chain env #11821

Merged
merged 5 commits into from
Feb 12, 2024

Conversation

ilija42
Copy link
Contributor

@ilija42 ilija42 commented Jan 19, 2024

This fixes some rest api responses getting omitted when the JAID was same across multiple chains

Copy link
Contributor

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

@ilija42 ilija42 marked this pull request as ready for review January 19, 2024 14:11
@ilija42 ilija42 marked this pull request as draft January 19, 2024 14:12
samsondav
samsondav previously approved these changes Jan 19, 2024
@ilija42 ilija42 marked this pull request as ready for review January 22, 2024 14:59
@ilija42 ilija42 requested a review from a team as a code owner January 22, 2024 14:59
@ilija42 ilija42 changed the title BCF-2834 Fix potential JAID collisions in multi chain env BCF-2901 Fix potential JAID collisions in multi chain env Jan 22, 2024
@ilija42 ilija42 requested review from samsondav and a team January 22, 2024 15:08
This fixes some rest api responses getting omitted when the JAID was same across multiple chains
# Conflicts:
#	integration-tests/actions/vrf/vrfv2plus/vrfv2plus_steps.go
#	integration-tests/actions/vrfv2_actions/vrfv2_steps.go
Comment on lines 168 to 169
func FormatWithPrefixedChainID(id, chainID string) string {
return fmt.Sprintf("%s/%s", chainID, id)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is kind of hard to read, and the opposite order could be surprising.
WDYT about inlining these cases? I think it will be less characters than this func name, and you can drop some explicit .String() calls too.

Copy link
Contributor Author

@ilija42 ilija42 Feb 12, 2024

Choose a reason for hiding this comment

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

Flipped the params, but not sure about inline. I feel like this stands out more and is less likely to make someone scratch their head if some test results needs to be prefixed. Although this is very simple so may not be an issue idk

@ilija42 ilija42 added this pull request to the merge queue Feb 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 12, 2024
@jmank88 jmank88 added this pull request to the merge queue Feb 12, 2024
Merged via the queue into develop with commit 1fb4203 Feb 12, 2024
93 checks passed
@jmank88 jmank88 deleted the fix-eth-keys-list branch February 12, 2024 19:28
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.

3 participants