Skip to content

Conversation

@lechangxu
Copy link

Milestone Delivery Checklist

  • The milestone-delivery-template.md has been copied and updated.
  • This pull request is being made by the same account as the accepted application.
  • I have disclosed any and all sources of reused code in the submitted repositories and have done my due diligence to meet its license requirements.
  • In case of acceptance, invoices must be submitted and payments will be transferred to the Polkadot AssetHub and/or fiat account provided in the application.
  • The delivery is according to the Guidelines for Milestone Deliverables.

Link to the application pull request: w3f/Grants-Program#2458

@keeganquigley keeganquigley self-assigned this Jan 7, 2026
@keeganquigley
Copy link
Contributor

Hi @lechangxu thanks for the delivery. I don't mean to be blunt here but there is much to be desired:

  • There are only 11 unit tests with 30% coverage. For core backend functionality, this should be much higher. Database operations don't seem to be tested at all (0% cov)
  • All of the main() functions are barely tested.
  • Why are the credentials hardcoded? Currently the readme exposes the password. Standard practice is to use .env variables.
  • Lines 22-27 in SQL_DB.py contain plaintext database credentials in comments
  • When running the Dockerfile, the .env file is in CAO/.env but the Dockerfile is looking for it in hacks/.env
  • .env file must be copied to multiple locations (CAO/ and hacks/)
  • Port 3306 hardcoded in multiple places
  • Conflicting packages: Both dotenv and python-dotenv in requirements.txt
  • What is the rationale for using print() instead of logging in a backend service?

Can you please implement some fixes here and get the code coverage up to at least 80%? Thanks.

@lechangxu
Copy link
Author

Hi @keeganquigley, Thank you very much for your thorough review and feedback. As this is our team’s first project, we acknowledge that there are indeed shortcomings in test coverage and standardization. We have fully understood the relevant standards and best practices, and we are actively improving our test cases, especially for core backend functionality and database operations. At the same time, we will promptly address the configuration and security issues you pointed out (such as environment variable management, hardcoded ports, and replacing print statements with logging). In our upcoming submissions, we will ensure that overall code coverage is raised to above 80%. Once again, thank you for your guidance—it is extremely valuable for our team’s growth.

@lechangxu
Copy link
Author

Hi @keeganquigley,

Thank you again for your thorough review and feedback. We’ve gone through each of the points and implemented fixes accordingly. Here’s a summary of the updates:

✅ Test coverage: Added unit tests for core backend functionality, including database operations. Overall coverage is now 80%+.

✅ main() functions: Introduced dedicated test cases to ensure proper validation of entry points.

✅ Credential management: Removed all hardcoded credentials. Environment variables are now consistently loaded via .env.

✅ SQL_DB.py comments: Cleaned up comments containing plaintext credentials.

✅ Dockerfile path: Fixed .env path issues. Only CAO/.env is required now, no duplication needed.

✅ Port configuration: Eliminated hardcoded 3306 values. Ports are configurable via environment variables.

✅ Dependencies: Unified to python-dotenv and removed duplicate/conflicting packages.

✅ Logging: Replaced print() statements with the standard logging module, supporting multiple log levels and outputs.

We’ll continue monitoring and refining to keep the project aligned with best practices. Your input has been invaluable in improving both quality and security, and we look forward to further collaboration.

@keeganquigley
Copy link
Contributor

Thanks @lechangxu can you also clarify which repos I should be using for testing M1? Should I use backend + frontend + database? Would you be able to create a Docker Compose file to spin up all three simultaneously or were you planning this for the next milestone?

@keeganquigley
Copy link
Contributor

Since M1 is for only the backend, I'll focus on that for now:

  • the readme only contains four basic commands with zero context. Can you add some descriptions?
  • The three repos still seem to have conflicting config instructions. There is no .env.example file.

Config management

If the default DSN is this:

func InitializeDB() {
    dsn := os.Getenv("DB_DSN")
    if dsn == "" {
        // MUST match the database setup instructions
        dsn = "queryweb3:!SDCsdin2df2@@tcp(127.0.0.1:3306)/QUERYWEB3?charset=utf8mb4&parseTime=True&loc=Local"
    }
    // ... rest of code
}

Can you create a backend/.env.example file:

# Database connection string
# Format: username:password@tcp(host:port)/database?params
DB_DSN=queryweb3:!SDCsdin2df2@@tcp(127.0.0.1:3306)/QUERYWEB3?charset=utf8mb4&parseTime=True&loc=Local

# Server port (default: 8082)
PORT=8082

# Environment (development, staging, production)
ENVIRONMENT=development

And then update all three READMEs to reference the same creds? Also, why three separate repos with no unified setup instructions? Any reason to not use a monorepo?

Error Handling

Line 85 in backend/model/yield.go uses panic in prod code. Can you replace this with a proper error return (i.e. "failed to scan yield data")

Testing

Unit tests look good. Consider adding e2e/integration tests for the next milestone. GraphQL schema looks reasonable.

@lechangxu
Copy link
Author

Hi @keeganquigley, thanks for your patience.
We’re currently preparing the Docker Compose setup to run backend, frontend, and database together. Please hold on for now — once it’s ready, we’ll share the documentation with you.

@lechangxu
Copy link
Author

Hi @keeganquigley,

We’re pleased to share that progress has been made on the setup:

Docker Compose configuration file: docker-compose.yaml

Updated Docker Compose documentation: quick-start.md

Please refer to these resources for the latest instructions. They will help you spin up the backend, frontend, and database together more smoothly. During the testing process, don’t hesitate to reach out if any questions or uncertainties arise — our team will respond promptly to provide clarification and support.

Once Milestone 1 is successfully completed, we will immediately move forward with Milestone 2. Through this progression, we aim to strengthen the Polkadot ecosystem in the field of applied data. This component serves as an application-layer data service while also forming a critical part of the infrastructure that supports the broader ecosystem.

Thank you for your collaboration and support.

@keeganquigley
Copy link
Contributor

Thanks for the fixes @lechangxu its getting there but still has issues that prevent me from being able to pass it. Please see my ongoing evaluation and make appropriate fixes. Thank you!

@lechangxu
Copy link
Author

Hi @keeganquigley,

Thanks for the detailed review — really appreciate the continued guidance. We’re going through your ongoing evaluation now and have already started addressing the remaining issues. Some of the items you highlighted (error handling, data validation, test coverage, and pipeline robustness) are now in our active task list, and we’ll be pushing updates accordingly.

In the meantime, we’ve also updated the README for Query-Web3/backend with clearer startup instructions and consistent environment variable documentation:
https://github.com/Query-Web3/backend/blob/main/README.md

And regarding your earlier question about monorepo vs. multi‑repo, we put together a short write‑up explaining why we’re not using a monorepo at this stage:
https://github.com/Query-Web3/backend/blob/main/docs/monorepo-drawbacks.md

We’ll continue working through the remaining items from your evaluation and follow up with fixes. Thanks again for the thorough feedback — it’s helping us tighten up the project significantly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants