Support fiber multiple hops payment in cross chain transaction

[contrun]

No description provided.

[quake]

There's no need to make a GenericStore abstraction of the store here, it violates the principle of composition over inheritance, you can just use a store + hook struct wherever you need a store callback.

[contrun]

Do you mean that the hooks can be passed to every one that needs them (instead of bundling them with the store)? There are many places in current code base which alters the status of an invoice, for example, https://github.com/contrun/fiber/blob/c53f6dfebba73ae7f9b3b75622abd7e8132eb3b6/src/rpc/invoice.rs#L340-L348 https://github.com/contrun/fiber/blob/c53f6dfebba73ae7f9b3b75622abd7e8132eb3b6/src/fiber/channel.rs#L1058-L1062 https://github.com/contrun/fiber/blob/c53f6dfebba73ae7f9b3b75622abd7e8132eb3b6/src/store/store.rs#L444-L447 , all the code listed above must have a reference to the InvoiceUpdateHook. So it seems to me define a

Struct StoreWithHook<IH: InvoiceUpdateHook> {
   store: Store,
   hook: IH,
}

and pass this StoreWithHook to above code would make life easier. That is essentially GenericStore<IH: InvoiceUpdateHook>.

The thing is that Hook and Store can't really work work independently. Hook (calling it StoreUpdateHook is better) should be part of Store because the hooks run when store is updated. We can't compose these two inter-dependent component.

[quake]

keep the original store implementation unchanged:

impl InvoiceStore for Store {
    fn insert_invoice(...) {
        ...
    }
}

and add a compsited store implementation:

struct InvoiceStoreWithHook {
    store: Store,
    invoice_hook: ConcreteInvoiceHook,
}

impl InvoiceStore for InvoiceStoreWithHook {
    fn insert_invoice(...) {
        self.store.insert_invoice(...);
        self.invoice_hook.on_invoice_updated(...);
    }
}

no need to introduce GenericStore struct and InvoiceUpdateHook trait

[contrun]

In https://github.com/contrun/fiber/tree/support-fiber-multiple-hops-payment-in-cross-chain-transaction-pass-hooks, I tried to use this approach (as I understood it). Can you confirm this align well with the approach you described? My code in that branch is very complicated, I don't know if that is the simplest way to accomplish what's on your mind. The complexity of my implementation is due to the following points:

1. Hooks has to be initialized in the main function (we need to have a singleton to receive all the updates and push them to the subscribers).
2. There are many modules which use InvoiceStore, so all of they have to call InvoiceStoreWithHooks::new, which is a function to initialize a InvoiceStore with a hook. This means we have to pass the invoice hooks created in the main function down to many modules.
3. Channel actors use ChannelActorStateStore along with InvoiceStore. So we have to implement ChannelActorStateStore for the InvoiceStoreWithHooks.
4. The same thing needs to be done for NetworkGraphStateStore (yeah, confusingly, this trait updates payment session), because we need to inject hooks for payments.

I didn't have a complete implementation yet. If my current code is what's on your mind. I can finish injecting payment hooks and removing the GenericStore.

[chenyukang]

applied_add_tlcs is introduced to resolve some concurrency issue, I'm not sure is it ok to remove it from here.

[contrun]

I think so. Because we are actually adding and removing the tlc from exactly the same function apply_add_tlc_operation_with_peeled_onion_packet.

[chenyukang]

seems ProcessingChannelError::FinalIncorrectPaymentHash does not return to caller. we should return the error at line 1019?

[contrun]

The store interface is a little bit confusing to me. Most of the store traits will never return errors. I don't know under what situation insert_payment_preimage may return an error (the current code does not return any error https://github.com/contrun/fiber/blob/fed2e63c7ec248d7319449d9bd7f388f3a943224/src/store/store.rs#L479-L488). It was my understanding that failure at this stage should be internal errors related to the store saving (not data validation error like the hashing result of this preimage does not match the hash). Can you recall why the InvoiceStore has Result as its return value? I thought all the business logic (data validation etc) should be done by the caller of the store trait (instead of in the store trait).

[chenyukang]

never mind, 1019 will return error, I got it wrong.
or you can remove the returning result for insert_payment_preimage by the way.

[contrun]

I changed this to FinalIncorrectPaymentHash anyway.