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

OrderBook Subgraph testing CI #36

Closed
wants to merge 172 commits into from
Closed

OrderBook Subgraph testing CI #36

wants to merge 172 commits into from

Conversation

NanezX
Copy link
Contributor

@NanezX NanezX commented Nov 1, 2023

No description provided.

Copy link
Contributor

@thedavidmeister thedavidmeister left a comment

Choose a reason for hiding this comment

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

other than the specific comments, high level concerns

  • The PR is far too large, it seems like this easily could have been split into smaller PRs, as it stands we have 68 thousand LOC to review in a single pass
  • the rust code is generally undocumented and untested
  • commented out code and typos makes the PR appear unfinished to me, probably a symptom of it being so large that you couldn't even check it properly yourself @NanezX

@@ -0,0 +1,76 @@
use colored::*;
Copy link
Contributor

Choose a reason for hiding this comment

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

don't have undifferentiated util modules


let full_cmd = format!("{} {}", main_cmd, args.join(" "));

println!("{} {}\n", "Running:".green(), full_cmd.blue());
Copy link
Contributor

Choose a reason for hiding this comment

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

use tracing not println

stdout_handle.join().expect("Failed to join stdout thread");

if status.success() {
println!("✅ {} {}\n", full_cmd.blue(), "completed".green());
Copy link
Contributor

Choose a reason for hiding this comment

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

use standard tracing and standard log levels, emojis and colours could cause issues for logging/monitoring tools

version: '3'
services:
evm_node:
image: vishalkale151071/hardhat-node
Copy link
Contributor

Choose a reason for hiding this comment

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

private docker, use org

- postgres
extra_hosts:
- host.docker.internal:host-gateway
environment:
Copy link
Contributor

Choose a reason for hiding this comment

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

hardcoded environment?

let end_point = "http://localhost:8020/";
let subgraph_name = "test/test";

let data = MapBuilder::new()
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we working with unstructured string interpolation? does serde_yaml not work?

std::env::current_dir().unwrap().display(),
root_dir
))
.args(&["-c", "npx graph codegen && npx graph build"])
Copy link
Contributor

Choose a reason for hiding this comment

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

npx??


pub struct Query;

impl Query {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this?


pub struct SyncStatus;

pub async fn wait() -> anyhow::Result<bool> {
Copy link
Contributor

Choose a reason for hiding this comment

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

wait for what?

@@ -0,0 +1,1252 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

why are generated files being committed to the repo?

@thedavidmeister
Copy link
Contributor

closing as this should be smaller PRs

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