feat(invoice): add new invoice creation function and dependencies #305
Conversation
|
Since RgbInvoice is currently a type alias for CallRequest, we can’t directly implement methods like new() on it. To address this, I propose refactoring it into a newtype wrapper (a zero-cost abstraction) with an explicit constructor |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.12 #305 +/- ##
======================================
- Coverage 4.9% 4.1% -0.7%
======================================
Files 11 13 +2
Lines 1298 1553 +255
======================================
+ Hits 63 64 +1
- Misses 1235 1489 +254
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dr-orlovsky
left a comment
There was a problem hiding this comment.
Can you please also fix the clippy lints?
| pub fn new( | ||
| scope: T, | ||
| beneficiary: RgbBeneficiary, | ||
| // Core parameters | ||
| value: Option<u64>, | ||
| expiry_time: Option<DateTime<Utc>>, | ||
| // Additional fields | ||
| api: Option<TypeName>, | ||
| call: Option<CallState>, | ||
| lock: Option<TinyBlob>, | ||
| endpoints: Option<impl Into<ConfinedVec<Endpoint, 0, 10>>>, | ||
| ) -> Self { |
There was a problem hiding this comment.
This is why I prefer (and rust guidelines) recommend builders in such cases: calls to functions with too many unnamed arguments (some of which will be just Nones) hard to read in the code. Clippy also grumbles about this.
Why you are not satisfied with the builder we have for the invoices?
There was a problem hiding this comment.
I think using the builder is totally ok, which is why this PR was initially marked as a draft
invoice/src/lib.rs
Outdated
| api: Option<TypeName>, | ||
| call: Option<CallState>, | ||
| lock: Option<TinyBlob>, | ||
| endpoints: Option<impl Into<ConfinedVec<Endpoint, 0, 10>>>, |
There was a problem hiding this comment.
this rather should be
| endpoints: Option<impl Into<ConfinedVec<Endpoint, 0, 10>>>, | |
| endpoints: impl IntoIterator<Endpoint>, |
And the method should panic if too many endpoints are provided
invoice/src/lib.rs
Outdated
| Bp(bp::ParseWitnessOutError), | ||
| } | ||
|
|
||
| pub struct RgbInvoice<T: Display + FromStr>(CallRequest<T, RgbBeneficiary>); |
There was a problem hiding this comment.
I think we still should have the default value here. Also we need derives
| pub struct RgbInvoice<T: Display + FromStr>(CallRequest<T, RgbBeneficiary>); | |
| #[derive(Clone, Eq, PartialEq, Debug, From)] | |
| #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | |
| #[cfg_attr( | |
| feature = "serde", | |
| serde( | |
| transparent, | |
| bound( | |
| serialize = "T: serde::Serialize, A: serde::Serialize", | |
| deserialize = "T: serde::Deserialize<'de>, A: serde::Deserialize<'de>" | |
| ) | |
| ) | |
| )] | |
| pub struct RgbInvoice<T: Display + FromStr = CallScope<ContractQuery>>(CallRequest<T, RgbBeneficiary>); |
|
I've added serde support and fixed constructor param issue |
|
@astral-bitlight is this PR is still actual and needed? |
|
We can closed it for now |
No description provided.