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: change put route put/ -> put #168

Merged
merged 7 commits into from
Oct 12, 2024
Merged

fix: change put route put/ -> put #168

merged 7 commits into from
Oct 12, 2024

Conversation

samlaf
Copy link
Collaborator

@samlaf samlaf commented Oct 4, 2024

Fixes Issue

Fixes #88

Changes proposed

Screenshots (Optional)

Note to reviewers

Updated to optimism 1.9.3 which required also updating to go1.22

@samlaf samlaf requested a review from epociask October 4, 2024 11:12
@samlaf
Copy link
Collaborator Author

samlaf commented Oct 4, 2024

cc @karlb for review. Just applied the patch you had shared with me here.

@samlaf samlaf requested a review from bxue-l2 October 4, 2024 11:13
@samlaf
Copy link
Collaborator Author

samlaf commented Oct 7, 2024

Not sure why the test is still failing. @karlb said he would take a look tomorrow.

Comment on lines +101 to +105
// we need to handle both: see https://github.com/ethereum-optimism/optimism/pull/12081
// /put is for generic commitments, and /put/ for keccak256 commitments
// TODO: we should probably separate their handlers?
mux.HandleFunc("/put", WithLogging(WithMetrics(svr.HandlePut, svr.m), svr.log))
mux.HandleFunc("/put/", WithLogging(WithMetrics(svr.HandlePut, svr.m), svr.log))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@epociask can you review that this comment and the routes make sense. tests are passing but still want to be 100% certain since I still don't get our routing logic with the different paths. Should probably document this api at some point..

Copy link
Collaborator

@bxue-l2 bxue-l2 left a comment

Choose a reason for hiding this comment

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

lgtm


require (
github.com/Layr-Labs/eigenda v0.8.4
github.com/consensys/gnark-crypto v0.12.1
github.com/ethereum-optimism/optimism v1.9.2
github.com/ethereum/go-ethereum v1.14.8
github.com/ethereum-optimism/optimism v1.9.4-0.20240927020138-a9c7f349d10b
Copy link
Collaborator

Choose a reason for hiding this comment

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

@samlaf Remember to change to a canonical tag after 1.9.4 is released by OP

@samlaf samlaf merged commit e0af9a4 into main Oct 12, 2024
7 checks passed
@samlaf samlaf deleted the fix-put-route branch October 12, 2024 09:18
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.

PUT endpoint without trailing slash does nothing
2 participants