Skip to content

Conversation

louisinger
Copy link
Collaborator

@louisinger louisinger commented Dec 9, 2021

This PR is a kind a "rebase" of #238. The idea is to isolate the domain refacto + redux-saga from 2of2 multisig account logic .

Account domain

Redux-saga

  • Redux saga allows replacing two middleware: redux-thunk and background-aliases (from webext-redux).
  • All asynchronous tasks are now managed by "sagas" (including the utxos and txs updater).
  • Thanks to saga, we can implement more complex logic (task cancelation or "worker pattern" for unspent updater).
  • I also noticed that saga is much more stable in the web extension context (saga runs in the background script).

Bug fix

  • fix the first address generation (I discovered that the problem comes from the edge case where lastUsedIndexes are -Infinity: now this is handled by the increment function in wallet reducer).

it closes #242
it closes #204

@bordalix @tiero please review

@bordalix bordalix mentioned this pull request Jan 24, 2022
@tiero
Copy link
Member

tiero commented Jan 24, 2022

With latest commit, it hangs (ie. keep loading) as soon I get in the choose fee screen

image

louisinger and others added 6 commits January 24, 2022 13:35
Explain line of code (browser.browserAction.openPopup() exists in Firefox but not in Chrome)
Taxi daemon via REST is returning assetAmount and assetSpread as string instead of number.
Temporary fix, waiting for this issue to be fixed:
vulpemventures/taxi-daemon#91
Note: clicking Receive and then on asset always worked
Comment on lines 61 to 63
if (!utxo) {
throw new Error(`blindPSET error: utxo not found '${input.hash.reverse().toString('hex')}'`);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be occasions where the blinding is either a) not done by us (ccoperative blinding) b) not blinded (like taxi transactions for example)

for a) it's fine to wait new PSETv2 specification that cleanly allows to do this, but for b) we must skip throwing error for unblinded inputs we do not control (ie. provided by taxi)

@louisinger

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick note: this code makes impossible to use Taxi, since first input from psetToUnsignedTx is from a utxo the wallet doesn't have (it comes from Taxi), so it will always throw in the case of a Taxi transaction.

I remove it and tested all type of transactions, and everything is working.

Just not sure if by removing it I'm forgetting any edge case. @louisinger ?

Copy link
Collaborator Author

@louisinger louisinger Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed throwing an error is not OK in case of unconfidential inputs (taxi especially). However, in case of confidential input we don't own, blinding will fail if we are not able to give it the blinders (utxo.blindingData).
Now, it's not a problem cause we always use Marina's inputs except the taxi ones but they are unconfidential. if in the future marina integrates a feature that uses confidential inputs from another wallet, it will be necessary to modify this function (add a parameter "externalInputsBlindingPrivateKeys" for instance). But I think it's better to wait for PSETv2 for this case as suggested by tiero (which would allow not to manage the blinding keys / blinders exchange).

TL;DR @bordalix no edge cases because Marina never handle cases of confidential inputs owned by another wallet. ACK to remove the check for me 👍

EDIT: I've pushed some comments on this function

@tiero
Copy link
Member

tiero commented Feb 1, 2022

Tried to make a send of USDt with fee paid in LBTC and got this error

I suspect is an explorer error when we try to broadcast. Would be better if we can show the exact error to user

image

@bordalix
Copy link
Contributor

bordalix commented Feb 1, 2022

This happens with me when I'm sending a Tx to blockstream that:

  • Is using same inputs sent on previous Tx, or
  • Is sending the same Tx (same hash) that was previously accepted

Waiting for 2 minutes should work, since it gives time for previous Tx to confirm and Marina to update list of utxos and Tx.

Having said that, I will raise the HTTP error to the UI.

@tiero
Copy link
Member

tiero commented Feb 1, 2022

Waiting for 2 minutes should work, since it gives time for previous Tx to confirm and Marina to update list of utxos and Tx.

was the first tx I made since long time, yes raising the full explorer error would be best

@tiero tiero mentioned this pull request Feb 1, 2022
@tiero tiero merged commit e206957 into vulpemventures:master Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Invalid balance and tx list on Chrome Fetch asset's data in transactions updater

3 participants