-
Notifications
You must be signed in to change notification settings - Fork 1k
Check grafting from pre 0.0.6 to 0.0.6 and later #5929
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
Conversation
5a462d9
to
d215fc8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test! Just a few minor comments
tests/tests/integration_tests.rs
Outdated
} | ||
} | ||
} | ||
let one_sec = time::Duration::from_secs(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be better as a constant
tests/tests/integration_tests.rs
Outdated
} | ||
} | ||
let one_sec = time::Duration::from_secs(1); | ||
thread::sleep(one_sec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not a big deal here, but you shouldn't use thread::sleep
in async code; use tokio::time::sleep
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For current usage is irrelevant as all tests need to wait, but if ever used elsewhere it could matter. Changing it.
tests/tests/integration_tests.rs
Outdated
@@ -796,6 +806,82 @@ async fn test_remove_then_update(ctx: TestContext) -> anyhow::Result<()> { | |||
Ok(()) | |||
} | |||
|
|||
async fn test_subgraph_grafting(ctx: TestContext) -> anyhow::Result<()> { | |||
async fn get_bloock_hash(block_number: i32) -> Option<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type 'bloock'
// We need to make sure that the preconditions for POI are fulfiled | ||
// namely that the anvil produces the proper block hashes for the | ||
// blocks of which we will check the POI | ||
assert_eq!(block_hash, block_hashes[(i - 1) as usize]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused by this - the comment made me think we would check anvil, but we are querying graph-node. Maybe the comment should just not mention anvil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. The 'blockchain' is more appropriate here,
@@ -0,0 +1,25 @@ | |||
specVersion: 0.0.5 | |||
description: Grafted Subgraph |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this naming confusing - 'grafted subgraph' implies that this subgraph is grafted onto something else. Maybe just calling it 'base subgraph' would be better. And what's currently called 'grafting subgraph' could then be called 'grafted subgraph', but leaving that as 'grafting subgraph' is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, those names are more intuitive.
This PR tries to check the consistency of the POI implementation for grafting of subgraph in case there is a transition from a spec version older than 0.0.6 towards the 0.0.6 or newer. The grafting block has a conversion of the legacy hashing algorithm to the new one, and here we test that this conversion did not change.
On order to keep the POI with the expected values, we need to make sure that anvil produces the same genesis block hence a fixed timestamp for starting it. Also both local start and remote one need to have same parameters. In addition to that we need to wait until the last checked grafted block is mined before we start the integration tests.
Moreover there is need for specifying an
index
parameter in the fetching POI query in order to avoid error when a local environment variableGRAPH_POI_ACCESS_TOKEN
is set as that would set it to zero address and produce inconsistent POI value.