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

fix: network conversions #264

Merged

Conversation

h4sh3d
Copy link
Member

@h4sh3d h4sh3d commented Jul 11, 2022

Fix #248

When reviewing #247 I missed the conversion flow, it should be blockchain::Network -> btc|xmr::Network and not the inverse. And the issue was not describing the problem correctly, sorry for that.

@h4sh3d h4sh3d added the bug Something isn't working label Jul 11, 2022
@h4sh3d h4sh3d added this to the v0.5.0 milestone Jul 11, 2022
@h4sh3d h4sh3d requested a review from LeoNero July 11, 2022 16:12
@@ -428,29 +428,42 @@ impl FromStr for Network {
}
}

impl From<bitcoin::Network> for Network {
Copy link
Member

Choose a reason for hiding this comment

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

Can we get both directions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we actually can. I rework the commits to not erase the work from @LeoNero as much as I can, thus I have to move the impl from the blockchain module into bitcoin. If this bothers you @LeoNero I can remove that part and you do another PR yourself. I should have catch that in your first PR but I didn't, sorry for that.

Copy link
Member

Choose a reason for hiding this comment

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

Let's merge this to keep the momentum going. Leo's work is accurately preserved here and if there is something bothering him with the PR here, he can always submit a fresh PR to add more docs/test/functionality.

Copy link
Member

Choose a reason for hiding this comment

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

Hi just saw that. There is no problem removing my implementation for things to work properly or for better code organization.

@h4sh3d h4sh3d force-pushed the feature/monero-blockchain-into branch from 18c6080 to a4998c4 Compare July 12, 2022 08:11
@h4sh3d h4sh3d force-pushed the feature/monero-blockchain-into branch from a4998c4 to c334930 Compare July 12, 2022 08:17
@TheCharlatan TheCharlatan merged commit d9a8bd7 into farcaster-project:main Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure correct impl From blockchain::Network for external network
3 participants