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

Misc documentation fix #1139

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Misc documentation fix #1139

wants to merge 2 commits into from

Conversation

jianoaix
Copy link
Contributor

@jianoaix jianoaix commented Jan 23, 2025

Why are these changes needed?

Checks

  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@jianoaix jianoaix requested review from pschork and samlaf January 23, 2025 03:13
@@ -309,7 +309,7 @@ func BatchFromProtobuf(proto *commonpb.Batch) (*Batch, error) {
type Attestation struct {
*BatchHeader

// AttestedAt is the time the attestation was made
// AttestedAt is the time the attestation was made in nanoseconds
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not change the variable name itself to AttestedAtNano

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's better to remain consistent with other timestamps in naming. I don't have objection if one wants to get all timestamps renamed by incorporating time units though.

core/v2/types.go Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these all auto-generated? Might be good to add to https://github.com/Layr-Labs/eigenda/blob/master/.gitattributes if so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are using swagger here

Comment on lines -575 to -578
},
"salt": {
"description": "Allow same blob to be dispersed multiple times within the same reservation period",
"type": "integer"
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Why were these docs not updated as part of the other PRs? Are these just hand crated? If not, should we have a ci to check that they have been regenerated correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should

Co-authored-by: Samuel Laferriere <[email protected]>
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