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 logic to compare compute setting to static counterparts, rather than evaluating static ComputeSetting directly #1108

Merged
merged 2 commits into from
Dec 4, 2024

Conversation

QuitCrypto
Copy link
Contributor

The existing logic evaluates static compute setting values as conditions for ternary expressions, which will always evaluate truthy. They should instead be comparing the existing compute setting to their static counterparts.

@QuitCrypto QuitCrypto force-pushed the main branch 2 times, most recently from 4fa73c1 to 1a1301c Compare December 4, 2024 21:43
@yargo13
Copy link
Contributor

yargo13 commented Dec 4, 2024

Nice catch, thanks! Can you generate a changeset with pnpm changeset ?

@yargo13 yargo13 changed the title Fix logic to compare compute setting to static counterparts, rather than evaluating static ComputeSetting directly 🧹 Fix logic to compare compute setting to static counterparts, rather than evaluating static ComputeSetting directly Dec 4, 2024
@yargo13
Copy link
Contributor

yargo13 commented Dec 4, 2024

Also saw that linter failed, fix would be

            const mapper =
                computeSetting === ComputeSetting.OnlyReduce
                    ? (this.logger.info('OnlyReduce setting is used. Skipping map step.'),
                      (r: RequestResponsePair) => r.response)
                    : (r: RequestResponsePair) => this.lzMap(compute, r, timeMarker)
            const reducer =
                computeSetting === ComputeSetting.OnlyMap
                    ? (this.logger.info('OnlyMap setting is used. Skipping reduce step.'),
                      (mappedResponses: string[]) => mappedResponses.join(''))
                    : (mappedResponses: string[]) => this.lzReduce(compute, cmd, mappedResponses, timeMarker)

Copy link
Contributor

@yargo13 yargo13 left a comment

Choose a reason for hiding this comment

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

Let's put it as patch. It also picked up another commit, but I think that is fine

@QuitCrypto
Copy link
Contributor Author

Re-marked it as a patch and rebased to sync it up with main again

@yargo13 yargo13 merged commit 486c69d into LayerZero-Labs:main Dec 4, 2024
5 checks passed
@yargo13 yargo13 changed the title 🧹 Fix logic to compare compute setting to static counterparts, rather than evaluating static ComputeSetting directly 🪲 Fix logic to compare compute setting to static counterparts, rather than evaluating static ComputeSetting directly Dec 5, 2024
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.

2 participants