fix: storage #1

sirily11 · 2025-10-31T05:11:29Z

No description provided.

Copilot

Pull Request Overview

This PR introduces centralized storage type constants and adds a factory function to create storage instances. The changes refactor hardcoded storage type strings into shared constants and provide a unified entry point for storage initialization.

Adds storage type constants (StorageClientTypeSQLite, StorageClientTypePostgres) in a new config file
Introduces a GetStorage factory function to create storage instances based on type
Updates existing code to use the new constants instead of hardcoded strings

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
internal/config/storage.go	Defines storage type constants for SQLite and Postgres
internal/config/shared_memory_keys.go	Adds a new key for storing the storage client instance
internal/contract/evm/storage/sql/storage.go	Implements the `GetStorage` factory function that creates storage instances
app/evm/storage/page.go	Refactors hardcoded storage type strings to use the new constants

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-10-31T05:14:40Z

internal/contract/evm/storage/sql/storage.go

+func GetStorage(storageType string, params ...any) (Storage, error) {
+	switch storageType {
+	case config.StorageClientTypeSQLite:
+		if len(params) == 0 || params[0] == nil {
+			return nil, fmt.Errorf("sqlite path is required")
+		}
+		sqlitePath, ok := params[0].(string)
+		if !ok {
+			return nil, fmt.Errorf("sqlite path must be a string")
+		}
+		return NewSQLiteDB(sqlitePath)
+	default:
+		return nil, fmt.Errorf("invalid storage type: %s", storageType)
+	}


The validation at line 52 is incorrect. According to NewSQLiteDB implementation (lines 257-263 of sqlite.go), an empty string is a valid value that defaults to $HOME/smart-contract-cli.db. The check params[0] == nil is sufficient, but the additional len(params) == 0 check should allow empty strings. Consider changing line 52 to if len(params) == 0 or removing the error for empty strings since NewSQLiteDB handles this case.

Copilot · 2025-10-31T05:14:40Z

internal/contract/evm/storage/sql/storage.go

 	UpdateConfig(id uint, config models.EVMConfig) (err error)
 	DeleteConfig(id uint) (err error)
 }
+


The GetStorage function lacks documentation. Add a comment explaining its purpose, the expected format of the params variadic argument for different storage types, and examples of usage.

Suggested change

// GetStorage returns a Storage implementation based on the provided storageType.

//

// The variadic params argument is used to pass additional configuration required by the storage type.

//

// Supported storage types and expected params:

// - config.StorageClientTypeSQLite: params[0] should be a string representing the SQLite database file path.

//

// Example usage:

// storage, err := GetStorage(config.StorageClientTypeSQLite, "/path/to/db.sqlite")

// if err != nil {

// // handle error

// }

//

Copilot · 2025-10-31T05:14:40Z

internal/contract/evm/storage/sql/storage.go

+		if !ok {
+			return nil, fmt.Errorf("sqlite path must be a string")
+		}
+		return NewSQLiteDB(sqlitePath)


The switch statement only handles SQLite but StorageClientTypePostgres is defined in the config. Consider adding a case config.StorageClientTypePostgres: that returns an appropriate error message indicating that Postgres support is not yet implemented, rather than treating it as an invalid storage type. This provides clearer feedback to users and makes future implementation more straightforward.

Suggested change

return NewSQLiteDB(sqlitePath)

return NewSQLiteDB(sqlitePath)

case config.StorageClientTypePostgres:

return nil, fmt.Errorf("Postgres support is not yet implemented")

Copilot

Pull Request Overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-01T13:29:33Z

tools/routergen/main.go

+	// Validate path to prevent directory traversal
+	cleaned := filepath.Clean(goModPath)
+	if strings.Contains(cleaned, "..") {
+		return "", fmt.Errorf("invalid file path: %s", goModPath)
+	}
+


The directory traversal check using strings.Contains(cleaned, \"..\") is insufficient. After filepath.Clean() normalizes the path, it removes .. segments that are within valid path components. This check will produce false positives for legitimate paths. Since goModPath is constructed from moduleRoot (a trusted input) and a constant string "go.mod", this validation is unnecessary and should be removed.

Suggested change

// Validate path to prevent directory traversal

cleaned := filepath.Clean(goModPath)

if strings.Contains(cleaned, "..") {

return "", fmt.Errorf("invalid file path: %s", goModPath)

}

Copilot · 2025-11-01T13:29:33Z

internal/contract/evm/abi/abi.go

+	// Validate filePath to prevent directory traversal
+	cleaned := filepath.Clean(filePath)
+	if strings.Contains(cleaned, "..") {


The directory traversal check is ineffective. filepath.Clean() already removes .. segments, so checking for \"..\" afterwards is meaningless. For proper directory traversal protection, you should check if the cleaned path is within an expected base directory using filepath.HasPrefix() or similar approach, not just check for \"..\" strings.

Copilot · 2025-11-01T13:29:33Z

.golangci.yml

@@ -1,221 +1,51 @@
+version: "2"


The golangci-lint configuration has been dramatically simplified, removing many important linter settings and exclusions. This removes critical configuration for varnamelen, godot, stylecheck, wrapcheck, and test file exclusions that were previously defined. This could cause linter failures or false positives. Ensure this simplification is intentional and doesn't break existing CI/CD pipelines.

Copilot · 2025-11-01T13:29:34Z

app/evm/storage/page.go

+	switch clientType {
+	case "sqlite":
 		key = config.StorageKeySqlitePathKey
-	} else if clientType == "postgres" {
+	case "postgres":
 		key = config.StorageKeyPostgresURLKey
-	} else {
+	default:
 		return fmt.Errorf("invalid client type: %s", clientType)
 	}


The switch statement uses string literals "sqlite" and "postgres" for comparison, but these should use the constants config.StorageClientTypeSQLite and config.StorageClientTypePostgres defined in the new config file for consistency and type safety.

Copilot · 2025-11-01T13:29:34Z

app/evm/storage/page.go

+	switch clientType {
+	case "sqlite":
 		key = config.StorageKeySqlitePathKey
-	} else if clientType == "postgres" {
+	case "postgres":
 		key = config.StorageKeyPostgresURLKey
-	} else {
+	default:
 		return fmt.Errorf("invalid client type: %s", clientType)
 	}


Similar to the previous switch statement, use the constants config.StorageClientTypeSQLite and config.StorageClientTypePostgres instead of string literals for consistency.

Copilot

Pull Request Overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-01T13:35:37Z

tools/routergen/main.go

+	// Validate path to prevent directory traversal
+	cleaned := filepath.Clean(goModPath)
+	if strings.Contains(cleaned, "..") {


The path traversal validation is ineffective. filepath.Clean() removes ".." components, so checking for ".." after cleaning will always be false. The validation should compare the cleaned path against the expected directory or validate before cleaning.

Copilot · 2025-11-01T13:35:37Z

internal/contract/evm/abi/abi.go

+	cleaned := filepath.Clean(filePath)
+	if strings.Contains(cleaned, "..") {
+		return nil, errors.NewABIError(errors.ErrCodeABIParseFailed, fmt.Sprintf("invalid file path: %s", filePath))
+	}


The path traversal validation is ineffective. filepath.Clean() removes ".." components, so checking for ".." after cleaning will always be false. The validation should compare the cleaned path against the expected directory or validate before cleaning.

Suggested change

cleaned := filepath.Clean(filePath)

if strings.Contains(cleaned, "..") {

return nil, errors.NewABIError(errors.ErrCodeABIParseFailed, fmt.Sprintf("invalid file path: %s", filePath))

}

absPath, err := filepath.Abs(filePath)

if err != nil {

return nil, errors.WrapABIError(err, errors.ErrCodeABIParseFailed, fmt.Sprintf("failed to resolve absolute path: %s", filePath))

}

wd, err := os.Getwd()

if err != nil {

return nil, errors.WrapABIError(err, errors.ErrCodeABIParseFailed, "failed to get working directory")

}

// Ensure absPath is within the working directory

rel, err := filepath.Rel(wd, absPath)

if err != nil {

return nil, errors.WrapABIError(err, errors.ErrCodeABIParseFailed, "failed to resolve relative path")

}

if strings.HasPrefix(rel, "..") || filepath.IsAbs(rel) {

return nil, errors.NewABIError(errors.ErrCodeABIParseFailed, fmt.Sprintf("invalid file path (outside working directory): %s", filePath))

}

Copilot · 2025-11-01T13:35:38Z

internal/contract/evm/contract/signer/privatekey_test.go

+	// Check for potential overflow before converting uint64 to int64
+	var gasUsedInt64 int64
+	const maxInt64 = 1<<63 - 1
+	if receipt.GasUsed > uint64(maxInt64) {
+		gasUsedInt64 = maxInt64 // Max int64 value
+	} else {
+		gasUsedInt64 = int64(receipt.GasUsed)
+	}
+	expectedMaxDecrease := new(big.Int).Add(amount, new(big.Int).Mul(big.NewInt(gasUsedInt64), gasFeeCap))


The overflow check silently caps gas to maxInt64, which could lead to incorrect calculations. If gas exceeds maxInt64, the test should fail or use big.Int directly: new(big.Int).SetUint64(receipt.GasUsed) instead of converting to int64.

Suggested change

// Check for potential overflow before converting uint64 to int64

var gasUsedInt64 int64

const maxInt64 = 1<<63 - 1

if receipt.GasUsed > uint64(maxInt64) {

gasUsedInt64 = maxInt64 // Max int64 value

} else {

gasUsedInt64 = int64(receipt.GasUsed)

}

expectedMaxDecrease := new(big.Int).Add(amount, new(big.Int).Mul(big.NewInt(gasUsedInt64), gasFeeCap))

// Use big.Int directly for gas used to avoid overflow issues

gasUsedBig := new(big.Int).SetUint64(receipt.GasUsed)

expectedMaxDecrease := new(big.Int).Add(amount, new(big.Int).Mul(gasUsedBig, gasFeeCap))

Copilot

Pull Request Overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-01T13:47:17Z

tools/routergen/main.go

+	// Validate path to prevent directory traversal
+	cleaned := filepath.Clean(goModPath)
+	if strings.Contains(cleaned, "..") {


The directory traversal check is insufficient. filepath.Clean normalizes paths, but checking for '..' after cleaning won't catch all traversal attempts. A cleaned absolute path like /etc/passwd would pass this check. Instead, validate that the cleaned path is within the expected directory using filepath.Rel or check if it has the expected prefix.

Copilot · 2025-11-01T13:47:17Z

.github/workflows/ci.yml

  lint-and-test:
    name: Lint, Format Check, and Test
-    runs-on: ubuntu-latest
+    runs-on: macos-latest


Switching from ubuntu-latest to macos-latest for CI significantly increases cost and runtime without clear justification in the PR. macOS runners are typically 10x more expensive and slower to provision. Unless there's a macOS-specific requirement, consider reverting to Ubuntu.

Suggested change

runs-on: macos-latest

runs-on: ubuntu-latest

fix: storage

7da9490

Copilot AI review requested due to automatic review settings October 31, 2025 05:11

Copilot AI reviewed Oct 31, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 1, 2025 13:26

sirily11 force-pushed the abi branch from 86a226c to 21a045d Compare November 1, 2025 13:26

Copilot AI reviewed Nov 1, 2025

View reviewed changes

sirily11 force-pushed the abi branch from 21a045d to a4a4c44 Compare November 1, 2025 13:31

Copilot AI review requested due to automatic review settings November 1, 2025 13:32

sirily11 force-pushed the abi branch from a4a4c44 to 2938901 Compare November 1, 2025 13:32

Copilot AI reviewed Nov 1, 2025

View reviewed changes

sirily11 force-pushed the abi branch from 2938901 to 1457de7 Compare November 1, 2025 13:39

fix: linting error

4904bfb

sirily11 force-pushed the abi branch from 1457de7 to 4904bfb Compare November 1, 2025 13:44

Copilot AI review requested due to automatic review settings November 1, 2025 13:44

sirily11 merged commit 2a89ebe into main Nov 1, 2025
6 checks passed

sirily11 deleted the abi branch November 1, 2025 13:47

Copilot AI reviewed Nov 1, 2025

View reviewed changes

+// GetStorage returns a Storage implementation based on the provided storageType.
+//
+// The variadic params argument is used to pass additional configuration required by the storage type.
+//
+// Supported storage types and expected params:
+//   - config.StorageClientTypeSQLite: params[0] should be a string representing the SQLite database file path.
+//
+// Example usage:
+//   storage, err := GetStorage(config.StorageClientTypeSQLite, "/path/to/db.sqlite")
+//   if err != nil {
+//       // handle error
+//   }
+//

-	cleaned := filepath.Clean(filePath)
-	if strings.Contains(cleaned, "..") {
-		return nil, errors.NewABIError(errors.ErrCodeABIParseFailed, fmt.Sprintf("invalid file path: %s", filePath))
-	}
+	absPath, err := filepath.Abs(filePath)
+	if err != nil {
+		return nil, errors.WrapABIError(err, errors.ErrCodeABIParseFailed, fmt.Sprintf("failed to resolve absolute path: %s", filePath))
+	}
+	wd, err := os.Getwd()
+	if err != nil {
+		return nil, errors.WrapABIError(err, errors.ErrCodeABIParseFailed, "failed to get working directory")
+	}
+	// Ensure absPath is within the working directory
+	rel, err := filepath.Rel(wd, absPath)
+	if err != nil {
+		return nil, errors.WrapABIError(err, errors.ErrCodeABIParseFailed, "failed to resolve relative path")
+	}
+	if strings.HasPrefix(rel, "..") || filepath.IsAbs(rel) {
+		return nil, errors.NewABIError(errors.ErrCodeABIParseFailed, fmt.Sprintf("invalid file path (outside working directory): %s", filePath))
+	}

fix: storage #1

fix: storage #1

Uh oh!

Conversation

sirily11 commented Oct 31, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants