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: fixed chunkIntoN to chunk correctly #6018

Merged
merged 2 commits into from
Oct 12, 2023

Conversation

rdvorkin
Copy link
Contributor

@rdvorkin rdvorkin commented Oct 3, 2023

chunkIntoN was chunking into N chunks, however it is used to chunk eth_getCode and eth_getProof responses into chunks of 2, so the desired action should be to chunk into chunks of length N https://github.com/ChainSafe/lodestar/blob/unstable/packages/prover/src/utils/evm.ts#L105

Because the current tests work on chunking array of length 4 into 2 the previous behavior was working and passing tests. When there are 6 requests the current behavior no longer works

chunkIntoN was chunking into N chunks, however it is used to chunk eth_getCode and eth_getProof responses into chunks of 2, so the desired action should be to chunk into chunks of length N
https://github.com/ChainSafe/lodestar/blob/unstable/packages/prover/src/utils/evm.ts#L105

Because the current tests work on chunking array of length 4 into 2 the previous behavior was working and passing tests. When there are 6 requests the current behavior no longer works
@rdvorkin rdvorkin requested a review from a team as a code owner October 3, 2023 11:25
@CLAassistant
Copy link

CLAassistant commented Oct 3, 2023

CLA assistant check
All committers have signed the CLA.

@rdvorkin rdvorkin changed the title FIX: fixed chunkIntoN to chunk correctly fix: fixed chunkIntoN to chunk correctly Oct 3, 2023
Comment on lines +1 to +14
import {expect} from "chai";
import {chunkIntoN} from "../../../src/utils/conversion.js";

describe("utils/conversion", () => {
describe("chunkIntoN", () => {
it("should return true if splitting into chunks correctly", async () => {
expect(chunkIntoN([1, 2, 3, 4, 5, 6], 2)).to.be.deep.eq([
[1, 2],
[3, 4],
[5, 6],
]);
});
});
});
Copy link
Member

Choose a reason for hiding this comment

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

I'd would suggest to add more test cases for this to ensure it works with different values of arr and n, I have not worked with this part of the code base so you likely know better what value combinations are relevant. Alternatively, if it is just used with a value n=2, it might make sense to simplify the function.

import {expect} from "chai";
import {chunkIntoN} from "../../../src/utils/conversion.js";

describe("utils/conversion", () => {
  describe("chunkIntoN", () => {
    const testCases: {arr: number[]; n: number; expected: number[][]}[] = [
      {
        arr: [1, 2, 3, 4, 5, 6],
        n: 2,
        expected: [
          [1, 2],
          [3, 4],
          [5, 6],
        ],
      },
    ];

    for (const {arr, n, expected} of testCases) {
      it(`should convert ${JSON.stringify(arr)} into chunks of ${n} as ${JSON.stringify(expected)}`, () => {
        expect(chunkIntoN(arr, n)).to.be.deep.eq(expected);
      });
    }
  });
});

@nazarhussain can provide better feedback on this

@nazarhussain nazarhussain self-assigned this Oct 11, 2023
@nazarhussain nazarhussain added the scope-light-clients All issues regarding light client development. label Oct 11, 2023
Copy link
Contributor

@nazarhussain nazarhussain left a comment

Choose a reason for hiding this comment

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

@rdvorkin That's seems a nice catch. Thanks for the PR.

Just add more tests cases as suggested by @nflaig and PR would be great. Also core more even n, odd n and lower than n cases.

@nazarhussain nazarhussain changed the base branch from unstable to nh/chunkinton-fix October 12, 2023 12:29
@nazarhussain nazarhussain merged commit b7ac520 into ChainSafe:nh/chunkinton-fix Oct 12, 2023
17 of 18 checks passed
nazarhussain pushed a commit that referenced this pull request Oct 12, 2023
* FIX: fixed chunkIntoN to chunk correctly

chunkIntoN was chunking into N chunks, however it is used to chunk eth_getCode and eth_getProof responses into chunks of 2, so the desired action should be to chunk into chunks of length N
https://github.com/ChainSafe/lodestar/blob/unstable/packages/prover/src/utils/evm.ts#L105

Because the current tests work on chunking array of length 4 into 2 the previous behavior was working and passing tests. When there are 6 requests the current behavior no longer works

* Fix strings in test
wemeetagain pushed a commit that referenced this pull request Oct 12, 2023
* fix: fixed chunkIntoN to chunk correctly (#6018)

* FIX: fixed chunkIntoN to chunk correctly

chunkIntoN was chunking into N chunks, however it is used to chunk eth_getCode and eth_getProof responses into chunks of 2, so the desired action should be to chunk into chunks of length N
https://github.com/ChainSafe/lodestar/blob/unstable/packages/prover/src/utils/evm.ts#L105

Because the current tests work on chunking array of length 4 into 2 the previous behavior was working and passing tests. When there are 6 requests the current behavior no longer works

* Fix strings in test

* Add more test cases

* Update test title

* Update packages/prover/test/unit/utils/conversion.test.ts

Co-authored-by: Nico Flaig <nflaig@protonmail.com>

* Update the test description

* Update the test description

* Update the test description

---------

Co-authored-by: Roman Dvorkin <121502696+rdvorkin@users.noreply.github.com>
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope-light-clients All issues regarding light client development.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants