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

show response commitment #144

Merged
merged 1 commit into from
Sep 22, 2024
Merged

show response commitment #144

merged 1 commit into from
Sep 22, 2024

Conversation

bxue-l2
Copy link
Collaborator

@bxue-l2 bxue-l2 commented Sep 20, 2024

Current behavior after put, it shows an empty string
Screenshot 2024-09-20 at 1 19 23 PM

This fix prints the response commitment
Screenshot 2024-09-20 at 1 36 23 PM

@@ -260,7 +260,7 @@ func (svr *Server) HandlePut(w http.ResponseWriter, r *http.Request) (commitment
}
}

svr.log.Info(fmt.Sprintf("write commitment: %x\n", comm))
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh yeah I noticed this as well. Not super sure what the intent of this was. If this is only meant for keccak commitments, then we should print it in the if clause above probably? Or was this just a mistake @epociask ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think there is a bit confusion on the term commitment schema, https://github.com/Layr-Labs/eigenda-proxy?tab=readme-ov-file#commitment-schemas. The readme section actually means returned commitment-schemas.

In the commented log above, it only prints comm used when you send data to eigenda-proxy. If it is optimism alt da, the commitment is left empty, so nothing gets printed.

I often find it useful to debug, if I have the response commitment, which is something rollup will commit onchain. So I am adding this change

Copy link
Collaborator

Choose a reason for hiding this comment

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

The responseCommitment should be same as comm when keccak is used right? So there's literally no point in logging comm? If that's the case, then let's just merge this. But if there is a difference between comm and responseCommit, then perhaps we should log both?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no really, for keccak, you append a version byte 0 before the hash, https://github.com/Layr-Labs/eigenda-proxy/blob/main/commitments/op.go#L110

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

since it is a byte, printing will just show 00 in the front

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh comm is the commit type?? I thought it was the commitment for keccak commitments, where the commitment is supplied in the POST request?

@samlaf samlaf merged commit 3f4f2ba into main Sep 22, 2024
7 checks passed
@samlaf samlaf deleted the fix-log-show-response-commitment branch September 22, 2024 01:35
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