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

Support eventstream example (bls_to_execution_change) from recent beacon API test suite #6167

Closed
jeluard opened this issue Dec 8, 2023 · 5 comments · Fixed by #6354
Closed
Labels
scope-testing Issues for adding test coverage, fixing existing tests or testing strategies.

Comments

@jeluard
Copy link
Contributor

jeluard commented Dec 8, 2023

Beacon APIs v2.4.0 introduce a new eventstream example that do not pass currently on lodestar.

  1) eventstream event data
       bls_to_execution_change:

      eventTestData[bls_to_execution_change] does not match spec's example
      + expected - actual

       {
         "message": {
      -    "from_bls_pubkey": "0x9048a71944feba4695ef870dfb5745c934d81c5efd934c0250a12942fcc2a2dfd6b20d53314379dec7aae5ca5fe9e9c4"
      -    "to_execution_address": "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
      +    "from_bls_pubkey": "0x933ad9491b62059dd065b560d256d8957a8c402cc6e8d8ee7290ae11e8f7329267a8811c397529dac52ae1342ba58c95"
      +    "to_execution_address": "0x9be8d619c56699667c1fedcd15f6b14d8B067f72"
           "validator_index": "1"
         }
         "signature": "0x1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505cc411d61252fb6cb3fa0017b679f8bb2305b26a285fa2737f175668d0dff91cc1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505"
       }
@jeluard jeluard added the scope-testing Issues for adding test coverage, fixing existing tests or testing strategies. label Dec 8, 2023
@jeluard jeluard changed the title Fix test failure from recent beacon API test suite Support eventstream example from recent beacon API test suite Dec 8, 2023
@nflaig
Copy link
Member

nflaig commented Dec 8, 2023

Updating test data here should fix this

[EventType.blsToExecutionChange]: ssz.capella.SignedBLSToExecutionChange.fromJson({
message: {
validator_index: "1",
from_bls_pubkey:
"0x9048a71944feba4695ef870dfb5745c934d81c5efd934c0250a12942fcc2a2dfd6b20d53314379dec7aae5ca5fe9e9c4",
to_execution_address: "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",

@jeluard jeluard changed the title Support eventstream example from recent beacon API test suite Support eventstream example (bls_to_execution_change) from recent beacon API test suite Dec 8, 2023
@jeluard
Copy link
Contributor Author

jeluard commented Dec 15, 2023

Updating the test data doesn't entirely fix the test. Namely, "to_execution_address": "0x9be8d619c56699667c1fedcd15f6b14d8B067f72" gets changed during the round-trip into "to_execution_address": "0x9be8d619c56699667c1fedcd15f6b14d8b067f72" (notice the only uppercase letter is now lower case).
hex encoding is not case aware and all letters are deserialized into lower-cases.

It's not yet clear what the best fix would be.

@nflaig
Copy link
Member

nflaig commented Dec 15, 2023

It's not yet clear what the best fix would be.

Let's just make the spec example address all lowercase?

@jeluard
Copy link
Contributor Author

jeluard commented Dec 16, 2023

Related to ethereum/beacon-APIs#395

@philknows
Copy link
Member

Would definitely prefer checksummed also as good standard practice. There's a lot of copying/pasting with these addresses and we should do our part to steward this practice. My preference is ethereum/beacon-APIs#396 over 395. Thanks for opening the PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope-testing Issues for adding test coverage, fixing existing tests or testing strategies.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants