feat(funding-service): collect deposits sent to the funding account - #2604
feat(funding-service): collect deposits sent to the funding account#2604SantiagoPittella wants to merge 4 commits into
Conversation
4b96675 to
ecd416b
Compare
4e184b7 to
93f7642
Compare
I think if the script is confirmed to be a P2ID script, then it should not be fallible to consume it as long as we know that it's targeting the service account ID. |
ecd416b to
6b76a4a
Compare
93f7642 to
3e9bef4
Compare
|
|
||
| /// Interval at which the service collects deposits sent to the funding account. | ||
| #[arg( | ||
| long = "top-up-interval", |
There was a problem hiding this comment.
Somewhat sounds like its topping itself up somehow.
I would consider reserve this phrasing for requesting funds from the faucet on devnet/testnet?
Perhaps p2id-collection-interval?
There was a problem hiding this comment.
Sounds good
| from_block: BlockNumber, | ||
| ) -> Result<SyncedNotes> { | ||
| let tip = self.chain_tip().await?; | ||
| // An empty range is rejected, and there is nothing to scan past the tip anyway. |
There was a problem hiding this comment.
I've always wondered if this isn't a bug tbh. Is an empty range request not just an empty response.
There was a problem hiding this comment.
Considering that it is inclusive in both ends, an empty range means that start > end, which doesn't makes much sense. I don't see why it shouldn't be an error
There was a problem hiding this comment.
Fair. I was thinking of rust ranges ie 7..3 is just an empty iterator
| .rpc_client | ||
| .clone() |
There was a problem hiding this comment.
I know you can get tonic to generate using Arc; I wonder if we shouldn't do that.
There was a problem hiding this comment.
I don't remember using it before, will take a look
| block_to: tip.as_u32(), | ||
| }), | ||
| prefix_len: NULLIFIER_PREFIX_LEN, | ||
| nullifiers: prefixes, |
There was a problem hiding this comment.
We should technically limit the size here?
There was a problem hiding this comment.
sounds good, will use the server side limit
|
|
||
| The scan restarts at the genesis block after a restart, because the service keeps nothing on disk. That costs one request: the node answers a whole block range in a single response, and returns only the blocks which hold a matching note. | ||
|
|
||
| Two choices in this collection are deliberate. It runs as its own transaction rather than riding along with a funding transaction, because a deposit note comes from outside the service and a note which turns out to be unconsumable must not be able to fail a request a client is waiting on. |
There was a problem hiding this comment.
I think this shouldn't matter - a p2id note should always be consumeable? Though perhaps not if it is recalled by the sender?
Perhaps that is not worth optimising around though, both should be rare, so its unlikely that a top up and a fund request coincide in time.
There was a problem hiding this comment.
also, it is simpler in the service side to have them split, since otherwise we would have two option:
a) request our inputs notes from the node on each funding tx
b) keep the collection of those notes in a separate task and add a channel to the worker
There was a problem hiding this comment.
@igamigo this works for your initial comment too
| } | ||
|
|
||
| /// Collects deposits until the service shuts down. | ||
| pub async fn run(mut self, shutdown: CancellationToken) -> Result<()> { |
There was a problem hiding this comment.
Will this not conflict with the funding service itself?
As in, we should probably have a single loop that handles both incoming requests, and top-up collections? Or at least a mutex to lock things into sequential execution and submission?
There was a problem hiding this comment.
The thing with incoming requests is that we need to scan the chain to get them. Though we can think of a way to merge them. Regarding the sequential execution: it is not really a problem because the account is built with rpc data, both executions will build the same account and the first one submitting will win the slot, making the other one to retry and that;s it
There was a problem hiding this comment.
I was thinking something like a separate scanning task that identifies p2id notes and sends them to the main task which includes them as part of the finding tx
There was a problem hiding this comment.
But overall this shouldn't matter much you're right
6b76a4a to
2a26ac9
Compare
|
@Mirko-von-Leipzig @igamigo due to the painfully large amount of conflicts, I opened #2615 which has the original change and the comments addressed in new commits |
Summary
Adds funding for the funder account via a public pay-to-ID note holding the native asset, and the service finds and consumes it on its own.
Only public, pay-to-ID, targeting the funding account, holding nothing but the native asset are collected.
Deposits are consumed in their own transaction, so a note that turns out to be unconsumable cannot fail a request a client is waiting on.
--top-up-intervalcontrols how often it runs, defaulting to one minute.Changelog