Skip to content

Implement scantxoutset method and test #164

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GideonBature
Copy link
Contributor

Going by the conversations with tcharding for PR #116 on implementing one method at a time for easier review: This is the fourth method implementation scantxoutset. Once this is approved, I’ll proceed with the next one.

@GideonBature GideonBature force-pushed the scantxoutset branch 3 times, most recently from 63e4f61 to 67b03b6 Compare May 12, 2025 23:49
@tcharding
Copy link
Member

Why is there identical code in the v17 and v18 blockchain modules of the client?

@GideonBature
Copy link
Contributor Author

Why is there identical code in the v17 and v18 blockchain modules of the client?

They basically work the same way, the difference is in their return type, v18 has an additional field (desc) in it's result, whereas it is lacking in v17. Or is there a way to reuse the client code in v17 for v18 and still be able to return the type that is meant for v18?

@tcharding
Copy link
Member

Bro I explained this to you already on the other two PRs you did. Perhaps go back over them and see if you can work it out.

@GideonBature
Copy link
Contributor Author

GideonBature commented May 14, 2025

Bro I explained this to you already on the other two PRs you did. Perhaps go back over them and see if you can work it out.

oh, I now understand, I was looking at how gettxout was used in the client, where despite the code is similar, they used it for v17 and again for v22. So I thought that for any change in the fields of return type of a method, there should also be a code in the module of the client for that particular version with the change. But I can see that it's not always the case.

For this recent push, I removed the repeated code across all versions having different fields in their type.

@tcharding
Copy link
Member

Oh well that would explain your confusion, the gettxout in v22 is just plain wrong. I'll remove it.

@tcharding
Copy link
Member

Done in #169. FTR bugs happen, if you see things that are wrong or look weird just ask (or PR to fix them). Apologies for being so fierce yesterday.

@tcharding
Copy link
Member

tcharding commented May 15, 2025

Returns column needs updating (and Notes).

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

Holy moly you picked a difficult one. I'm going to have to come back to this when I can put more thought into it. Feel free to ignore my suggestions until then.

/// > 2. "scanobjects" (array, required) Array of scan objects
#[derive(Deserialize, Debug, Clone, PartialEq)]
#[serde(untagged)]
pub enum ScanTxOutSet {
Copy link
Member

Choose a reason for hiding this comment

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

Oh this is interesting. This is a new idea for the crate, it could be done for all the methods that have verbose results. I'm not sure it adds any value though. I'd just remove this and leave the three individual return types. Plus I'd document 'returned when the foo action as taken'.

Copy link
Member

Choose a reason for hiding this comment

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

after looking at v28 I'm not sure - leave it with me for a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright... Noted. That would be great using it for the methods having verbose results.

@GideonBature
Copy link
Contributor Author

Done in #169. FTR bugs happen, if you see things that are wrong or look weird just ask (or PR to fix them). Apologies for being so fierce yesterday.

No problem. I will do this going forward.

@GideonBature
Copy link
Contributor Author

Returns column needs updating (and Notes).

Noted!

@GideonBature
Copy link
Contributor Author

Holy moly you picked a difficult one. I'm going to have to come back to this when I can put more thought into it. Feel free to ignore my suggestions until then.

Noted....

@GideonBature
Copy link
Contributor Author

Returns column needs updating (and Notes).

for the notes, is there any need to change it? since it describes perfectly that the scantxoutset is experimental.

@tcharding
Copy link
Member

Oh you are right, lets leave the notes as they are.

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