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.

Sign up for GitHub

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

Jump to bottom

Implement estimateRouteFee #193

Merged
merged 1 commit into from
Jan 5, 2025
Merged

Implement estimateRouteFee #193

merged 1 commit into from
Jan 5, 2025

Conversation

bnonni
Copy link
Contributor

@bnonni bnonni commented Jan 3, 2025

PR for Issue #167

@bnonni bnonni marked this pull request as ready for review January 3, 2025 18:07
@bnonni
Copy link
Contributor Author

bnonni commented Jan 3, 2025

@alexbosworth upgraded the draft PR to ready for review for the sake of submitting it. However, I still need to get the tests working and passing for estimate_fee_route. Could use your eyes / advisement since the tests are custom suite.

@bnonni
Copy link
Contributor Author

bnonni commented Jan 4, 2025

@alexbosworth tests are passing. this is ready for final review.

@bnonni bnonni force-pushed the implement-estimateroutefee branch from ddd75a0 to 498e04e Compare January 4, 2025 02:03
@bnonni bnonni changed the title Implement estimateroutefee Implement estimateRouteFee Jan 4, 2025
@bnonni bnonni force-pushed the implement-estimateroutefee branch 2 times, most recently from eb711ea to 36ed141 Compare January 4, 2025 02:07
alexbosworth
alexbosworth reviewed Jan 4, 2025
View reviewed changes
lnd_methods/index.js Outdated Show resolved Hide resolved
@bnonni bnonni force-pushed the implement-estimateroutefee branch from 3aeba2d to 99128f3 Compare January 4, 2025 21:06
@alexbosworth
Copy link
Owner

how about renaming it to getRoutingFeeEstimate? seems like that would match getChainFeeEstimate

@bnonni
Copy link
Contributor Author

bnonni commented Jan 4, 2025

Yeah that works. For clarity, rename everything that is currently named estimateRouteFee to getRoutingFeeEstimate?

@alexbosworth
Copy link
Owner

I think that would be more along the lines of the existing naming yeah, getRoutingFeeEstimate, i also though about like getOffchainFeeEstimate but I think routing fee has more clarity

@bnonni
Copy link
Contributor Author

bnonni commented Jan 4, 2025

Yeah i agree. Routing fee makes more sense to me as well. Sounds good. Changing that now.

@bnonni bnonni force-pushed the implement-estimateroutefee branch from a5d3eba to 6c50d1e Compare January 4, 2025 22:53
@bnonni
Copy link
Contributor Author

bnonni commented Jan 4, 2025

@alexbosworth completed and ready for another review.

@bnonni bnonni force-pushed the implement-estimateroutefee branch from 621627f to 750d74d Compare January 4, 2025 23:42
@alexbosworth alexbosworth merged commit 750d74d into alexbosworth:master Jan 5, 2025
@bnonni bnonni deleted the implement-estimateroutefee branch January 5, 2025 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexbosworth alexbosworth alexbosworth left review comments

Assignees

@bnonni bnonni

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bnonni @alexbosworth