vulpemventures / marina

Liquid Wallet browser extension
MIT License
39 stars 19 forks source link

Marina CustomScript account #345

Closed louisinger closed 2 years ago

louisinger commented 2 years ago

This PR implements the new provider described here: https://github.com/vulpemventures/marina-provider/pull/20

It lets marina to send and receive funds on descriptors-based accounts.

Custom script account

The main change is the new CovenantAccount, implementing the Account interface via a new IdentityInterface. The CovenantIdentity is using a descriptors-like language to describe the account script type.

utxos and transactions updating is already handled by the current version of updater. The major change about this is the new selectedAccount member in the marina provider: it lets to implement new marina.useAccount, then updater and restorer in background work like the MainAccount ones. In Popup UI, "click on update" will launch an update for all the accounts.

Accounts are identified by their namespace or AccountID. The main account ID is a constant: "main". The covenant account ID aims to derive the wallet master key (using slip13). Thus, with mnemonic + namespace + template(s), the user is able to restore its covenants accounts.

User interface changes

This PR does not modify the UX but some changes are introduced by "multiple accounts paradigm":

How to test

ecclib.ts trick to bump LDK

TL;DR

In order to support BIP341 (or segwitv1) changes, we need to bump the latest LDK lib. This new version requires the injection of tiny-secp256k1 lib in some packages. The import * as ecc from 'tiny-secp256k1'; is doing that job.

However, the secp256k1 implementation works with wasm and marina extension is building three JS contexts: popup, background and content. for security reasons it is not possible to inject .wasm files directly in content scripts. That's why we need to import base64 wasm (.wasm files encoded as a base64 string which can be embedded in the javascript files).

To replace import 'tiny-secp256k1' by the "base64 wasm string" we are using the src/ecclib.ts file + webpack aliases. This is totally transparent for developers (import 'tiny-secp256k1` like the other dependencies).

it closes #320 it closes #322 it closes #321 it closes #318 it closes #325

@tiero @bordalix please review

louisinger commented 2 years ago

Independently of the network chosen, the extension is always trying to access http://localhost:8000/assets (taxi regtest url).

Right. I was thinking we do that already on master, will remove the regtest update on non-regtest networks

bordalix commented 2 years ago

Independently of the network chosen, the extension is always trying to access http://localhost:8000/assets (taxi regtest url).

Right. I was thinking we do that already on master, will remove the regtest update on non-regtest networks

Just to be clear, we do pool periodically (1 minute) taxi url. But we do it to the corresponding taxi url for the chosen network. The issue here is that it is ignoring the chosen network and always pooling the regtest url.

tiero commented 2 years ago
  1. Build from this branch
yarn && yarn build && yarn web-ext:build
  1. Tried restore an old menmonic, I got

burst exit include cactus remain orange stomach flash daring rabbit mandate web

image

PS: Same thing happens with "New Wallet" and click on "Remind me later"

tiero commented 2 years ago

WRT which account to use for change when cross-spending utxos accross accounts?

Best solution so far is to use the account chosen with useAccount API, below the equivalent UI component

Home empty - drop down menu

tiero commented 2 years ago

Other feedback on UI: best to "hide" it in settings, switching address type/account is not really something user want to do often.

Also we should show the current used account in the receive tab and in send flow ("we using this $account for the change address)

tiero commented 2 years ago

@louisinger https://user-images.githubusercontent.com/3596602/169332441-326f4624-7363-4ab0-a5f4-20480f91c4fb.mov

tiero commented 2 years ago

Is Accounts tab in UI setting binded to actual account selector? I switched from test to main, and await marina.getSelectedAccount() is still returning metest

tiero commented 2 years ago

I get invalid template on marina.importTemplate("eltr($test, { asm($test OP_CHECKSIG), asm(OP_FALSE) })")

louisinger commented 2 years ago

Is Accounts tab in UI setting binded to actual account selector? I switched from test to main, and await marina.getSelectedAccount() is still returning metest

the settings/account selector let you choose the changeAccount not the "selected one". selectedAccount is a "local" variable in the webapp context, so each webapp has a different selectedAccount

tiero commented 2 years ago

There is some problems with incrementing the index of the derivation path, when calling importTemplate right before getNextAddress: if you try call th importTemplate and getNextAddress multiple time will return always the same index 1

louisinger commented 2 years ago

related PR: https://github.com/louisinger/marina/pull/1

tiero commented 2 years ago

on MainAccount f I do await marina.getAccountInfo() will get Uncaught Error: Account undefined not found