Closed gianfra-t closed 3 months ago
@pendulum-chain/devs I believe there is still a bit of polishing to be done, and certainly lot of testing. But since this involves heavy changes on the logic and on how we deal with recovery scenarios, I think this is in a state we can review this decisions and discuss about them, before moving forward.
@TorstenStueber now that you have addressed the requested changes in the review, is this PR ready-ready or do you still need to finish testing? Please leave a comment once that is done, I just want to make sure we progress fast as soon as everything is done.
Also calling out the dependency here, Kacper's UI developments are blocked as most of the logic for the UI would be unblocked by merging this PR
I added the final commit. All manual testing works for me now.
I also added a testing configuration for mocking sep24 and removing the minimum transfer amount – this makes manual testing easier.
One main change in the flow was that I moved the creation of the Stellar ephemeral account right to the beginning of the flow – even before the squid router transactions are executed. This is because we can't create and sign the other Stellar transactions unless this happened, otherwise there is not sequence number available for the ephemeral account.
This is strictly necessary for the other security ticket (dumping all necessary data), where all relevant data (all signed transactions) should be dumped before the flow starts.
Looks good to me!
@TorstenStueber I only have one comment regarding this but it is not critical for the protoype. I understand why you moved the creation of the stellar account otherwise it would not be possible to create the offramp transactions in constructInitialState
, but wouldn't it be better to do so (both the creation and operations) once the user has signed the squid router transaction and committed the funds?
@ebma I saw this error when I ran lint:ts
, but I ignored it as I figured it already existed before this PR.
Doesn't it occur because two competing modules pollute the global namespace and put a conflicting ethereum
object into window
?
Apparently it is metamask that injects the ethereum
object into window
and these two modules need to access it and for this purpose provide type definitions for it.
@gianfra-t That is a good point and I thought about this quite a bit when I changed that.
In my view there is just no ideal solution here (at least not on the prototype level):
I chose the latter option because the risk there is one we already started to take since the first prototype.
@ebma I solved the yarn build
problem by removing the metamask related code. It was not used anywhere in the project and it was the only one that required the packages @chainsafe/metamask-polkadot-adapter
and @chainsafe/metamask-polkadot-types
so that I removed them.
General
Center the off-ramp code around one main hook, that contains the function selector logic based on the operation status.
Every time the operation status is refreshed, the corresponding next step is triggered, which after completion will set the next state and store it in case the application is closed. Example nabla swap.
Also adds a recovery hook to read initially the states of the local storage. Assuming the application was closed on the middle of a process, this will set the
OperationStatus
to the last completed one, triggering again the operation that was shut down.The rest of changes deal with handling the re-triggering of operations, and possible paths to verify/avoid re-sending transactions that were successful sent but not registered.
Specific modifications for recovery handling around operations
For each operation, we need to analyze what are the effects of a re-computation for the scenario in which the operation was cut on the middle. For example,
OperationStatus.PendulumEphemeralReady
will trigger nabla'sapprove
. The most likely scenario is that, if the application closes, it will do so while waiting for the contract call to be in the block. We need to decide on each operation either to check if it was actually successful, re-try no matter what, or handle a potential re-try error.Sep10
This is done as soon as the user clicks offramp. In case of crash, there is no issue to restart from scratch.
Sep24
Will start with the token obtained on sep10. Stellar ephemeral will be recovered. Since (at least for Mykobo) the url to make the offramp has only one use, we need to start this again.
Squidrouter approvals
Here we need to be careful not to send already accepted transactions. For this reason the corresponding logic was modified as follows:
Check ephemeral ready
This operation checks token and native balance funds of the pendulum ephemeral account. We can easily read the expected balance to avoid further computation in case of recovery.
Nabla Approval
Already checks if an approval is required. Also, in case the scenario the transaction is in progress right when the app is recovering, re-sending this transaction would not break the process (TODO this logic needs to be added)
Nabla Swap
We can also check if the expected funds are on the ephemeral to avoid sending the transaction again. TODO: Similarly to the approval, for the scenario in which the recovery happens just as the transaction is still getting finalized, we would re-send a transaction that will fail. This could be handled by checking if we are in recovery mode, and waiting for corresponding funds to be on the ephemeral, otherwise send the transaction again. Second failure would indicate that there is something wrong.
Note: it would be easier to use the hash but polkadot.js does not allow for this.
Stellar ephemeral creation (funding)
If operation is incomplete, I believe it is better to ask again the signing service for the signed creation operation. This is because it will get the newest sequence number. Otherwise, we may run the risk that the sequence is outdated.
Stellar Operations
Aside from storing the operations, if the app shuts down while receiving the signature, we ask the signing service for newly signed operations again.
Redeem Operation
We check the stellar balance of the ephemeral account, or for the case of redeeming transaction is being finalized, we handle in a custom manner the error of re-sending the transaction by checking the funds of the stellar account directly. We do not have the required id to listen to the event in this case.
For a second (more likely) scenario, in which the operation is cut while listening to the redeem event, we could also store the redeem event restart the waiting process. But I believe waiting for the funds in stellar covers both cases.
Offramp transaction
We handle the specific bad sequence error which would happen if we re-send the transaction after the previous one was validated. We ignore this case (if we are in recovery mode)
Cleaning transactions
Nothing is done, and transactions would be sent again. Since this is the last operation there is no damage in doing so (they would be defunded anyway).