Skip to content

Conversation

@natobritto
Copy link
Contributor

From v26 onward, the return type of scanblocks RPC has an argument called completed, this PR adds it and handles into type conversion and in previous versions changes.

Partially fixes issue #474.

Copy link
Collaborator

@jamillambert jamillambert left a comment

Choose a reason for hiding this comment

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

It would be good to add an assert to the existing test that completed is returned for v26 and later. I checked the test for v26 and completed is true.

@natobritto
Copy link
Contributor Author

Also included testing assert to ensure the completed field presence is verified from v26 onward.

@natobritto natobritto force-pushed the fix-scanblocks branch 2 times, most recently from 0c2f508 to 39dfa65 Compare January 26, 2026 20:38
jamillambert
jamillambert previously approved these changes Jan 26, 2026
Copy link
Collaborator

@jamillambert jamillambert left a comment

Choose a reason for hiding this comment

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

ACK 39dfa65

{
assert!(json.completed);
}
}
Copy link
Member

@tcharding tcharding Jan 27, 2026

Choose a reason for hiding this comment

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

Perhaps this is better?

    let model: Result<mtype::ScanBlocksStart, ScanBlocksStartError> = json.into_model();
    let model = model.unwrap();

    let _: Option<ScanBlocksStatus> = node.client.scan_blocks_status().expect("scanblocks status");

    let _: ScanBlocksAbort = node.client.scan_blocks_abort().expect("scanblocks abort");

    #[cfg(not(feature = "v25_and_below"))]
    {
        assert!(model.completed.is_some());
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented, modified the model part for lint errors.

@tcharding
Copy link
Member

Reviewed: 39dfa65

@tcharding tcharding closed this Jan 27, 2026
@tcharding
Copy link
Member

Hit wrong button.

Comment on lines 502 to 503
let model: Result<mtype::ScanBlocksStart, ScanBlocksStartError> = json.into_model();
model.unwrap();
let model: mtype::ScanBlocksStart = json.into_model().unwrap();
let _ = &model;
Copy link
Member

Choose a reason for hiding this comment

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

We want to keep the error type here mate. The reason we do this which is admittedly strange is to make sure we exported the error type in the right place. We had bugs before we used this pattern when we forgot to do the re-exports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, introduced back in the type with slight name deviation for the lint.

@tcharding
Copy link
Member

Reviewed 38e407d

From v26 onward, the return type of `scanblocks` RPC has an argument
called `completed`, this commit adds it and handles into conversions.

#[cfg(not(feature = "v25_and_below"))]
{
assert!(_model.completed.is_some());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a big fan of this _ just to get around the lint. Would it be better to assert something for all versions above this feature gated assert like assert!(model.from_height <= model.to_height)?

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we shouldn't use the under score, that has a specific meaning.

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.

3 participants