use new ExtranonceAllocator APIs#11
Conversation
1169233 to
23b685d
Compare
|
|
||
| #[derive(uniffi::Object)] | ||
| pub struct Sv2ExtranoncePrefix { | ||
| inner: Mutex<Option<AllocatedExtranoncePrefix>>, |
There was a problem hiding this comment.
Can we remove Option here, and keep it just AllocatedExtranoncePrefix, and then pass it to new methods as owned, instead of having an Arc?
There was a problem hiding this comment.
hmm I guess you want to bring the ownership pattern from stratum-mining/stratum#2098 into here (de-allocate automatically by dropping)
the main thing to keep in mind here is that UniFFI objects always cross the language boundary as handles/pointers, so they do not model move-only Rust ownership directly
I get how the Option seems a bit awkward at first, but we need a one-shot consume step on the Rust side, which is why the wrapper stores Option<AllocatedExtranoncePrefix> and uses take_inner() to consume it exactly once.
if this were a pure-Rust API, passing AllocatedExtranoncePrefix by value would make sense, but with the current UniFFI object boundary that would require a broader API redesign (e.g.: Sv2ExtranonceAllocator instantiating channels, which tbh feels like a terrible idea).
I'm adding comments to explain the reasoning behind Option, but I would prefer that we avoid broader API redesigns.
| self.extranonce_allocator = Sv2ExtranonceAllocator( | ||
| local_prefix_bytes=static_prefix, | ||
| total_extranonce_len=FULL_EXTRANONCE_SIZE, | ||
| max_channels=MAX_CHANNELS, |
There was a problem hiding this comment.
Very interesting that CI didn't flagged this
| max_channels=MAX_CHANNELS, | |
| max_channels=POOL_MAX_CHANNELS, |
There was a problem hiding this comment.
fixed and added 9801d35 to run some examples on CI
full example coverage on CI is non-trivial and out of scope
066e07f to
9801d35
Compare
Shourya742
left a comment
There was a problem hiding this comment.
We can merge after CI is fixed
9801d35 to
6132d70
Compare
close #3
also removes the obsolete
RequestedMaxTargetOutOfRangefrom stratum-mining/stratum#2118