feat!: eigenda client confirmation depth #821
Conversation
samlaf
force-pushed
the
samlaf/feat--eigenda-client-confirmation-depth
branch
from
fbb1851 to
99d213c
Compare
| if err != nil { | ||
| return nil, fmt.Errorf("failed to dial ETH RPC node: %w", err) | ||
| } | ||
| edasmCaller, err = edasm.NewContractEigenDAServiceManagerCaller(common.HexToAddress(config.SvcManagerAddr), ethClient) |
There was a problem hiding this comment.
Does this PR make cert verification mandatory?
There was a problem hiding this comment.
No, but I still made the eth-rpc and service-manager address mandatory, because I realized we should probably be checking the confirmed status of the batch onchain when the disperser tells us (defensive programming).
Realized that it didn't make sense to require them only when confirmationDepth > 0, because why also not verify that the batch has landed onchain when confirmationDepth = 0 (meaning its included only in the currentBlock)? See
eigenda/api/clients/eigenda_client.go
Line 270 in 7f7fdaf
There was a problem hiding this comment.
@epociask is that OK with you or do you see a problem with this?
There was a problem hiding this comment.
Talked offline. We are fine with this. Added a breaking change comment to the PR description.
style: fix lint problem (comparison to bool constant) docs: better documentation for EigenDAClientConfig.WaitForConfirmationDepth tests: add eda client e2e test for waitForConfirmationDepth feature
They weren't required which caused a bug
We still need it in eigenda-proxy until we upstream all the verification functionality to eigenda_client
samlaf
force-pushed
the
samlaf/feat--eigenda-client-confirmation-depth
branch
from
35e2cca to
50afc56
Compare
samlaf
changed the title
Closes EBE-107.
Why are these changes needed?
Starting to upstream some of the eigenda-proxy features to eigenda-client.
This PR is needed for Layr-Labs/eigenda-proxy#187
Did not add a unit test, as I'm not a big fan of mock-based tests, which is the only pattern we have currently to test the eigenda-client. Added an e2e test on holesky instead, but this does not seem to be checked by any of our ci workflows... PR incoming (#820 ).
BREAKING CHANGES:
Checks