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

DataAPI V2 swagger refactor #1062

Closed
wants to merge 6 commits into from
Closed

Conversation

pschork
Copy link
Contributor

@pschork pschork commented Jan 2, 2025

Why are these changes needed?

Swagger needs the server handlers defined within a subpackage v1 or v2 in order to isolate handler discovery. This is a pretty big initial refactor to make that work. Subsequent refactors will happen post landing to consolidate shared handlers.

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 :(

@pschork pschork requested a review from jianoaix January 2, 2025 07:38
@pschork pschork force-pushed the pschork/dataapi_v2_swagger_sub branch from be6d12f to 2cb1dde Compare January 2, 2025 22:51
@pschork pschork force-pushed the pschork/dataapi_v2_swagger_sub branch from 2cb1dde to e0c187e Compare January 2, 2025 22:55
@pschork pschork force-pushed the pschork/dataapi_v2_swagger_sub branch from e0c187e to 4380aca Compare January 2, 2025 23:40
// Copy the operator id to a 32 byte array.
copy(operatorId[:], operator.OperatorId)

operatorInfo, err := sc.api.QueryOperatorInfoByOperatorIdAtBlockNumber(ctx, operator.OperatorId, uint32(operator.BlockNumber))

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types High

Incorrect conversion of an unsigned 64-bit integer from
strconv.ParseUint
to a lower bit size type uint32 without an upper bound check.

Copilot Autofix AI 27 days ago

To fix the problem, we need to ensure that the value of operator.BlockNumber is within the range of a 32-bit unsigned integer before performing the conversion. This can be done by adding an upper bound check using math.MaxUint32. If the value exceeds this limit, we should handle the error appropriately.

Suggested changeset 1
disperser/dataapi/v2/subgraph_client.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/disperser/dataapi/v2/subgraph_client.go b/disperser/dataapi/v2/subgraph_client.go
--- a/disperser/dataapi/v2/subgraph_client.go
+++ b/disperser/dataapi/v2/subgraph_client.go
@@ -376,2 +376,9 @@
 
+		if operator.BlockNumber > math.MaxUint32 {
+			operatorIdString := "0x" + hex.EncodeToString(operatorId[:])
+			errorMessage := fmt.Sprintf("block number exceeds uint32 limit: %d for operator %s", operator.BlockNumber, operatorIdString)
+			addOperatorWithErrorDetail(operators, operator, operatorId, errorMessage)
+			sc.logger.Warn(errorMessage)
+			continue
+		}
 		operatorInfo, err := sc.api.QueryOperatorInfoByOperatorIdAtBlockNumber(ctx, operator.OperatorId, uint32(operator.BlockNumber))
EOF
@@ -376,2 +376,9 @@

if operator.BlockNumber > math.MaxUint32 {
operatorIdString := "0x" + hex.EncodeToString(operatorId[:])
errorMessage := fmt.Sprintf("block number exceeds uint32 limit: %d for operator %s", operator.BlockNumber, operatorIdString)
addOperatorWithErrorDetail(operators, operator, operatorId, errorMessage)
sc.logger.Warn(errorMessage)
continue
}
operatorInfo, err := sc.api.QueryOperatorInfoByOperatorIdAtBlockNumber(ctx, operator.OperatorId, uint32(operator.BlockNumber))
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
operatorInfo, err := sc.api.QueryOperatorInfoByOperatorIdAtBlockNumber(ctx, operator.OperatorId, uint32(operator.BlockNumber))
if err != nil {
operatorIdString := "0x" + hex.EncodeToString(operatorId[:])
errorMessage := fmt.Sprintf("query operator info by operator id at block number failed: %d for operator %s", uint32(operator.BlockNumber), operatorIdString)

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types High

Incorrect conversion of an unsigned 64-bit integer from
strconv.ParseUint
to a lower bit size type uint32 without an upper bound check.

Copilot Autofix AI 27 days ago

To fix the problem, we need to add an upper bound check before converting the 64-bit unsigned integer to a 32-bit unsigned integer. This ensures that the value fits within the range of a 32-bit unsigned integer and prevents unexpected values.

  • We will add a check to ensure that the value of operator.BlockNumber does not exceed the maximum value of a 32-bit unsigned integer (math.MaxUint32).
  • If the value is within the valid range, we will proceed with the conversion. Otherwise, we will handle the error appropriately.
Suggested changeset 1
disperser/dataapi/v2/subgraph_client.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/disperser/dataapi/v2/subgraph_client.go b/disperser/dataapi/v2/subgraph_client.go
--- a/disperser/dataapi/v2/subgraph_client.go
+++ b/disperser/dataapi/v2/subgraph_client.go
@@ -9,2 +9,3 @@
 	"time"
+	"math"
 
@@ -376,2 +377,9 @@
 
+		if operator.BlockNumber > math.MaxUint32 {
+			operatorIdString := "0x" + hex.EncodeToString(operatorId[:])
+			errorMessage := fmt.Sprintf("block number exceeds uint32 range: %d for operator %s", operator.BlockNumber, operatorIdString)
+			addOperatorWithErrorDetail(operators, operator, operatorId, errorMessage)
+			sc.logger.Warn(errorMessage)
+			continue
+		}
 		operatorInfo, err := sc.api.QueryOperatorInfoByOperatorIdAtBlockNumber(ctx, operator.OperatorId, uint32(operator.BlockNumber))
EOF
@@ -9,2 +9,3 @@
"time"
"math"

@@ -376,2 +377,9 @@

if operator.BlockNumber > math.MaxUint32 {
operatorIdString := "0x" + hex.EncodeToString(operatorId[:])
errorMessage := fmt.Sprintf("block number exceeds uint32 range: %d for operator %s", operator.BlockNumber, operatorIdString)
addOperatorWithErrorDetail(operators, operator, operatorId, errorMessage)
sc.logger.Warn(errorMessage)
continue
}
operatorInfo, err := sc.api.QueryOperatorInfoByOperatorIdAtBlockNumber(ctx, operator.OperatorId, uint32(operator.BlockNumber))
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
indexedOperatorInfo, err := dataapi.ConvertOperatorInfoGqlToIndexedOperatorInfo(operatorInfo)
if err != nil {
operatorIdString := "0x" + hex.EncodeToString(operatorId[:])
errorMessage := fmt.Sprintf("failed to convert operator info gql to indexed operator info at blocknumber: %d for operator %s", uint32(operator.BlockNumber), operatorIdString)

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types High

Incorrect conversion of an unsigned 64-bit integer from
strconv.ParseUint
to a lower bit size type uint32 without an upper bound check.

Copilot Autofix AI 27 days ago

To fix the problem, we need to ensure that the uint64 value parsed from the BlockNumber does not exceed the maximum value that a uint32 can hold before performing the conversion. This can be done by adding a bounds check before the conversion. If the value exceeds the maximum uint32 value, we should handle it appropriately, such as by logging an error or using a default value.

Suggested changeset 1
disperser/dataapi/v2/subgraph_client.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/disperser/dataapi/v2/subgraph_client.go b/disperser/dataapi/v2/subgraph_client.go
--- a/disperser/dataapi/v2/subgraph_client.go
+++ b/disperser/dataapi/v2/subgraph_client.go
@@ -8,2 +8,3 @@
 	"strconv"
+	"math"
 	"time"
@@ -376,3 +377,12 @@
 
-		operatorInfo, err := sc.api.QueryOperatorInfoByOperatorIdAtBlockNumber(ctx, operator.OperatorId, uint32(operator.BlockNumber))
+		var blockNumber uint32
+		if operator.BlockNumber > math.MaxUint32 {
+			errorMessage := fmt.Sprintf("block number %d exceeds uint32 max value", operator.BlockNumber)
+			addOperatorWithErrorDetail(operators, operator, operatorId, errorMessage)
+			sc.logger.Warn(errorMessage)
+			continue
+		} else {
+			blockNumber = uint32(operator.BlockNumber)
+		}
+		operatorInfo, err := sc.api.QueryOperatorInfoByOperatorIdAtBlockNumber(ctx, operator.OperatorId, blockNumber)
 		if err != nil {
EOF
@@ -8,2 +8,3 @@
"strconv"
"math"
"time"
@@ -376,3 +377,12 @@

operatorInfo, err := sc.api.QueryOperatorInfoByOperatorIdAtBlockNumber(ctx, operator.OperatorId, uint32(operator.BlockNumber))
var blockNumber uint32
if operator.BlockNumber > math.MaxUint32 {
errorMessage := fmt.Sprintf("block number %d exceeds uint32 max value", operator.BlockNumber)
addOperatorWithErrorDetail(operators, operator, operatorId, errorMessage)
sc.logger.Warn(errorMessage)
continue
} else {
blockNumber = uint32(operator.BlockNumber)
}
operatorInfo, err := sc.api.QueryOperatorInfoByOperatorIdAtBlockNumber(ctx, operator.OperatorId, blockNumber)
if err != nil {
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options

operators[operatorId] = &QueriedOperatorInfo{
IndexedOperatorInfo: indexedOperatorInfo,
BlockNumber: uint(operator.BlockNumber),

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types High

Incorrect conversion of an unsigned 64-bit integer from
strconv.ParseUint
to a lower bit size type uint without an upper bound check.
func addOperatorWithErrorDetail(operators map[core.OperatorID]*QueriedOperatorInfo, operator *Operator, operatorId [32]byte, errorMessage string) {
operators[operatorId] = &QueriedOperatorInfo{
IndexedOperatorInfo: nil,
BlockNumber: uint(operator.BlockNumber),

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types High

Incorrect conversion of an unsigned 64-bit integer from
strconv.ParseUint
to a lower bit size type uint without an upper bound check.
if err != nil {
return nil, err
}
quorumNumbers = append(quorumNumbers, uint8(quorum))

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types High

Incorrect conversion of an integer with architecture-dependent bit size from
strconv.Atoi
to a lower bit size type uint8 without an upper bound check.
Incorrect conversion of an integer with architecture-dependent bit size from
strconv.Atoi
to a lower bit size type uint8 without an upper bound check.

Copilot Autofix AI 27 days ago

To fix the problem, we need to ensure that the conversion from int to uint8 is safe. This can be done by adding bounds checking to ensure that the value is within the valid range for uint8 before performing the conversion. Specifically, we should check that the value is between 0 and 255 inclusive.

  1. Modify the code to include bounds checking for the conversion from int to uint8.
  2. If the value is out of bounds, handle the error appropriately (e.g., return an error).
Suggested changeset 1
disperser/dataapi/v2/subgraph_client.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/disperser/dataapi/v2/subgraph_client.go b/disperser/dataapi/v2/subgraph_client.go
--- a/disperser/dataapi/v2/subgraph_client.go
+++ b/disperser/dataapi/v2/subgraph_client.go
@@ -509,2 +509,5 @@
 			}
+			if quorum < 0 || quorum > 255 {
+				return nil, fmt.Errorf("quorum value out of range: %d", quorum)
+			}
 			quorumNumbers = append(quorumNumbers, uint8(quorum))
EOF
@@ -509,2 +509,5 @@
}
if quorum < 0 || quorum > 255 {
return nil, fmt.Errorf("quorum value out of range: %d", quorum)
}
quorumNumbers = append(quorumNumbers, uint8(quorum))
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
parsed[i] = &OperatorQuorum{
Operator: string(opq.Operator),
QuorumNumbers: quorumNumbers,
BlockNumber: uint32(blockNum),

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types High

Incorrect conversion of an unsigned 64-bit integer from
strconv.ParseUint
to a lower bit size type uint32 without an upper bound check.

Copilot Autofix AI 27 days ago

To fix the problem, we need to ensure that the value of blockNum fits within the 32-bit unsigned integer range before converting it to uint32. This can be done by adding a check to verify that blockNum is less than or equal to math.MaxUint32. If the value exceeds this limit, we should handle the error appropriately.

Suggested changeset 1
disperser/dataapi/v2/subgraph_client.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/disperser/dataapi/v2/subgraph_client.go b/disperser/dataapi/v2/subgraph_client.go
--- a/disperser/dataapi/v2/subgraph_client.go
+++ b/disperser/dataapi/v2/subgraph_client.go
@@ -6,2 +6,20 @@
 	"fmt"
+	"math"
+	"sort"
+	"strconv"
+	"time"
+
+	"github.com/Layr-Labs/eigenda/core"
+	"github.com/Layr-Labs/eigenda/disperser/dataapi"
+	"github.com/Layr-Labs/eigenda/disperser/dataapi/subgraph"
+
+	"github.com/Layr-Labs/eigensdk-go/logging"
+
+	"github.com/gammazero/workerpool"
+)
+
+import (
+	"context"
+	"encoding/hex"
+	"fmt"
 	"sort"
@@ -514,3 +532,8 @@
 			QuorumNumbers:  quorumNumbers,
-			BlockNumber:    uint32(blockNum),
+			BlockNumber:    func() uint32 {
+				if blockNum > math.MaxUint32 {
+					panic(fmt.Sprintf("blockNum %d exceeds uint32 range", blockNum))
+				}
+				return uint32(blockNum)
+			}(),
 			BlockTimestamp: blockTimestamp,
EOF
@@ -6,2 +6,20 @@
"fmt"
"math"
"sort"
"strconv"
"time"

"github.com/Layr-Labs/eigenda/core"
"github.com/Layr-Labs/eigenda/disperser/dataapi"
"github.com/Layr-Labs/eigenda/disperser/dataapi/subgraph"

"github.com/Layr-Labs/eigensdk-go/logging"

"github.com/gammazero/workerpool"
)

import (
"context"
"encoding/hex"
"fmt"
"sort"
@@ -514,3 +532,8 @@
QuorumNumbers: quorumNumbers,
BlockNumber: uint32(blockNum),
BlockNumber: func() uint32 {
if blockNum > math.MaxUint32 {
panic(fmt.Sprintf("blockNum %d exceeds uint32 range", blockNum))
}
return uint32(blockNum)
}(),
BlockTimestamp: blockTimestamp,
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
}

return &BatchNonSigningInfo{
BlockNumber: uint32(confirmBlockNum),

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types High

Incorrect conversion of an unsigned 64-bit integer from
strconv.ParseUint
to a lower bit size type uint32 without an upper bound check.

Copilot Autofix AI 27 days ago

To fix the problem, we need to ensure that the value parsed as a 64-bit unsigned integer does not exceed the maximum value for a 32-bit unsigned integer before converting it. This can be done by adding an upper bound check before the conversion. If the value exceeds the maximum allowable value for a 32-bit unsigned integer, we should handle it appropriately, such as returning an error or using a default value.

Suggested changeset 1
disperser/dataapi/v2/subgraph_client.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/disperser/dataapi/v2/subgraph_client.go b/disperser/dataapi/v2/subgraph_client.go
--- a/disperser/dataapi/v2/subgraph_client.go
+++ b/disperser/dataapi/v2/subgraph_client.go
@@ -545,2 +545,6 @@
 	}
+	// Check if confirmBlockNum exceeds the maximum value for uint32
+	if confirmBlockNum > math.MaxUint32 {
+		return nil, fmt.Errorf("confirmBlockNum exceeds the maximum value for uint32: %d", confirmBlockNum)
+	}
 	nonSigners := make([]string, len(infoGql.NonSigning.NonSigners))
EOF
@@ -545,2 +545,6 @@
}
// Check if confirmBlockNum exceeds the maximum value for uint32
if confirmBlockNum > math.MaxUint32 {
return nil, fmt.Errorf("confirmBlockNum exceeds the maximum value for uint32: %d", confirmBlockNum)
}
nonSigners := make([]string, len(infoGql.NonSigning.NonSigners))
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
return &BatchNonSigningInfo{
BlockNumber: uint32(confirmBlockNum),
QuorumNumbers: quorums,
ReferenceBlockNumber: uint32(blockNum),

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types High

Incorrect conversion of an unsigned 64-bit integer from
strconv.ParseUint
to a lower bit size type uint32 without an upper bound check.

Copilot Autofix AI 27 days ago

To fix the problem, we need to ensure that the value parsed as a uint64 does not exceed the maximum value that can be held by a uint32 before performing the conversion. This can be done by adding a bounds check to verify that the parsed value is within the acceptable range for a uint32.

The best way to fix this issue is to add a check to ensure that the uint64 value is less than or equal to math.MaxUint32 before converting it to uint32. If the value exceeds this limit, we should handle the error appropriately.

Suggested changeset 1
disperser/dataapi/v2/subgraph_client.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/disperser/dataapi/v2/subgraph_client.go b/disperser/dataapi/v2/subgraph_client.go
--- a/disperser/dataapi/v2/subgraph_client.go
+++ b/disperser/dataapi/v2/subgraph_client.go
@@ -553,3 +553,8 @@
 		QuorumNumbers:        quorums,
-		ReferenceBlockNumber: uint32(blockNum),
+		ReferenceBlockNumber: func() uint32 {
+			if blockNum > math.MaxUint32 {
+				panic(fmt.Sprintf("blockNum %d exceeds uint32 range", blockNum))
+			}
+			return uint32(blockNum)
+		}(),
 		NonSigners:           nonSigners,
EOF
@@ -553,3 +553,8 @@
QuorumNumbers: quorums,
ReferenceBlockNumber: uint32(blockNum),
ReferenceBlockNumber: func() uint32 {
if blockNum > math.MaxUint32 {
panic(fmt.Sprintf("blockNum %d exceeds uint32 range", blockNum))
}
return uint32(blockNum)
}(),
NonSigners: nonSigners,
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@pschork pschork marked this pull request as ready for review January 3, 2025 02:09
@pschork pschork closed this Jan 3, 2025
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.

1 participant