refactor: remove refundable state

[m1sterc001guy]

See https://github.com/fedimint/fedimint/issues/4014 for motivation.

I believe the Refundable state is unnecessary in the lightning client state machine and can be removed. This eliminates a state transition, but also eliminates the arbitrary 30 second timeout on payments.

[justinmoon]

What kind of migration would be required for this? If a client is in Refundable, can we just stick them in another state?

[m1sterc001guy]

What kind of migration would be required for this? If a client is in Refundable, can we just stick them in another state?

It should be possible to transition the old client back from Refundable to Funded, since Funded has a superset of transitions that Refundable did (i.e the transitions that Refundable used to have are now moved to Funded).

Once client side migrations have been implemented of course :)

I think the PR is a bit premature since client side migrations are not implemented yet, but wanted to give an example of what I was thinking to fix this problem https://github.com/fedimint/fedimint/issues/4014. I think this should help with some of the problems the Mutiny guys are hitting.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 81.72043% with 17 lines in your changes are missing coverage. Please review.

:exclamation: No coverage uploaded for pull request base (master@e08dd65). Click here to learn what that means. Report is 10 commits behind head on master.

:exclamation: Current head 86cd86f differs from pull request most recent head b78baee. Consider uploading reports for the commit b78baee to get more accurate results

Files	Patch %	Lines
modules/fedimint-ln-client/src/pay.rs	75.86%	14 Missing :warning:
fedimint-load-test-tool/src/common.rs	0.00%	2 Missing :warning:
fedimint-wasm-tests/src/lib.rs	0.00%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #4024 +/- ## ========================================= Coverage ? 57.63% ========================================= Files ? 193 Lines ? 42868 Branches ? 0 ========================================= Hits ? 24705 Misses ? 18163 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[m1sterc001guy]

Maybe we can add a "likely to be refunded" flag to the Funded state and set it when the GW is doing things it shouldn't do? It wouldn't influence the SM, but could be used to give users more clarity.

Yep, good point. Perhaps the refund height can go here too. Like you said, I think ultimately the cancelled flag on the contract should be the source of truth, but we can perhaps give the end user some additional info on what is happening.

I'm going to put this aside for the moment and try to finish state machine db migrations so that this can be a backwards-compatible change.

[m1sterc001guy]

Note: @joschisan's NG lightning implementation does the same thing as this PR https://github.com/fedimint/fedimint/pull/3468

[m1sterc001guy]

This should be ready for review. I think this makes our lightning payments simpler.

I tried to do this in the most backwards compatible way possible, so I left some states in enums that can be cleaned up later. I wrote a db migration to add an error_reason to the Refund state, but if we won't want that, the db migration isn't required.

Writing a test for the db migration is currently kind of difficult since we need to keep old structs around. https://github.com/fedimint/fedimint/issues/4536

[elsirion]

This will require cleanup after the fact to get rid of no longer used states after #4536 lands.

[elsirion]

Also, I vote against backporting this. It fixes a minor bug but is a rather complex change that could massively delay 0.3.

[m1sterc001guy]

If we're not going to backport, we will need to wait to merge until after we fix https://github.com/fedimint/fedimint/issues/4700. Otherwise it will make fixing that issue painful

[elsirion]

@m1sterc001guy want to merge it now? The #4700 fix doesn't seem to have resulted in any conflicts.

[m1sterc001guy]

I think I'm close to have a solution for https://github.com/fedimint/fedimint/issues/4536, in which case I would prefer to get rid of the new enums introduced here. If we're not going to backport this, let's wait a little longer so I can clean it up :)

[justinmoon]

dev call: don't put it into 0.3.0. Try to backport to 0.3.1. @m1sterc001guy going to remove need to duplicate enums before merging.

[m1sterc001guy]

Converting to draft until https://github.com/fedimint/fedimint/pull/4719 is merged

[m1sterc001guy]

Going to work on https://github.com/fedimint/fedimint/issues/4823 first, since this will make the migrations easier to write.

[m1sterc001guy]

This is finally ready for another round of review.