Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI: Improve Adding/deleting recipients in Send , and move the recipient to a webcomponent #1782

Conversation

relativisticelectron
Copy link
Collaborator

@relativisticelectron relativisticelectron commented Jun 30, 2022

Follow up on: #1747

image

TODO:

  • Restructure the innerHTML code into a web component
  • Adding, and deleting of recipients
  • sort out tests and cypress
  • remove console.logs after review (this will allow fast debugging if needed)

Notable changes:

  • The webcomponent recipient-box has a proptery value which contains a dictionary like:
{
    unit: "btc",
    amount: 2,
    recipient_id: 1,
    address: "bcrt1qfvkcy2keql72s8ev87ek93uxuq3xxsx9l0n03r",
    label: "",
    amount_input: "2",
    btc_amount: 2
}
  • All values of all recipient-boxes are then combined before form submission to a dictionary, jsonified and stored in an hidden input recipient_dicts and then available in request.form (Other methods of making the fields available for request.form did not work and are less elegant.). This will allow in the future to unify some code from PsbtCreator.paymentinfo_from_ui and PsbtCreator.paymentinfo_from_json

Breaking API changes:
Changed subtract_from to start from 0, relevant for paymentinfo_from_json see

def paymentinfo_from_json(cls, specter, wallet, request_json):
. Original:
"subtract_from": subtract_from - 1,

@moneymanolis
Copy link
Collaborator

moneymanolis commented Jul 26, 2022

I like the structure in d6c8689 - excellent work overall, @relativisticelectron. I just have one comment (see above).

@relativisticelectron
Copy link
Collaborator Author

I like the structure in d6c8689 - excellent work overall, @relativisticelectron. I just have one comment (see above).

Thanks. It was my first web component. Thanks for all the feedback; it helped me a lot.

@moneymanolis
Copy link
Collaborator

@relativisticelectron I made a few comments for final optimisations. Great solution with 85a4777. No we can just keep fillform as it is. This will definitely be used in an extension etc. Thanks for all the work!

@relativisticelectron
Copy link
Collaborator Author

I tested the fillform with
Peek 2022-07-28 16-30

@moneymanolis : Could you test the liquid unit changer?

@moneymanolis
Copy link
Collaborator

I tested the fillform

Looks good!

@moneymanolis : Could you test the liquid unit changer?

OK, will do!

@moneymanolis
Copy link
Collaborator

moneymanolis commented Jul 29, 2022

I've tested with a Elements Regtest node: The asset selector works if we take the highlighted second condition out of this line:
{% if specter.is_liquid and wallet.balance.get("assets", {}) %}

Looks like this then:

grafik

I suppose the condition means that there are no other "assets" than LBTC in the wallet (because if there are there would be more to select from). I will double check with Stepan on how to proceed, he has implemented all the Liquid stuff and I still don't (want to? :-)) grasp it.

EDIT: Also works with {% if specter.is_liquid and wallet.balance["assets"] == {} %}

Not sure whether we need 10-8 here:

grafik

@relativisticelectron
Copy link
Collaborator Author

relativisticelectron commented Jul 29, 2022

OK. :-) That is as far as I got too... I was able to setup an elements node (in regtest), but "generatetoaddress" just didn't work. So I was unable to create funds or other assets. Also linking elements to the bitcoin regtest node failed for me.....

@moneymanolis
Copy link
Collaborator

moneymanolis commented Jul 29, 2022

OK. :-) That is as far as I got too... I was able to setup an elements node (in regtest), but "generatetoaddress" just didn't work. So I was unable to create funds or other assets. Also linking elements to the bitcoin regtest node failed for me.....

You can run elements "in isloation" (not connected to Bitcoin Core) if you use validatepegin=0 in the elements.conf.
Yeah, generatetoaddress doesn't work for me either. But, there are also just faucets for liquidtestnet to play around with assets and LBTC / to have some of them.

I am just posting a sample conf here (the testnet part is from a blockstream link):

# Running elementsd without chain parameter will run regtest
chain=elementsregtest

validatepegin=0
server=1
rpcuser=michaelsaylor
rpcpassword=mstr

# Regtest settings
[elementsregtest]
rpcport=18884

# Testnet settings:
[liquidtestnet]
rpcport=18891
txindex=1
anyonecanspendaremine=0
initialfreecoins=2100000000000000
con_dyna_deploy_start=0
con_max_block_sig_size=150
checkblockindex=0
addnode=liquid.network:18444
addnode=liquid-testnet.blockstream.com:18892
addnode=liquidtestnet.com:18891
fallbackfee=0.00000100
con_has_parent_chain=0
parentgenesisblockhash=NULL
pubkeyprefix=36
scriptprefix=19
blindedprefix=23
bech32_hrp=tex
blech32_hrp=tlq
pchmessagestart=410edd62
dynamic_epoch_length=1000
signblockscript=51210217e403ddb181872c32a0cd468c710040b2f53d8cac69f18dad07985ee37e9a7151ae
evbparams=dynafed:0:::

EDIT: Link to a faucet: https://liquidtestnet.com/faucet

@moneymanolis
Copy link
Collaborator

moneymanolis commented Jul 29, 2022

After talking with Stepan, we need to ensure that the we keep the status quo on master, which is:

Show BTC/sat if there are no assets, and show select box with all assets if they are any, which looks like this:

grafik

I can check it tomorrow.

@relativisticelectron
Copy link
Collaborator Author

Thanks for the elements config. I can now send tractors and the unit selector works well.
image

@moneymanolis
Copy link
Collaborator

moneymanolis commented Jul 29, 2022

Yes, the PR works in the same way as on master, but I am wondering whether it would not be better to have at least a separate unit logic if a Liquid wallet has ONLY LBTC (probably the most likely case for a user currently), I just hacked it to show:

grafik

Basically an elif ({% elif specter.is_liquid and wallet.balance["assets"] == {} %}) in addAssetSelectorToShadowRoot()
plus adjusting updateUnitLabelAndStep()

IMO this could also be done in a separate PR. Just checking whether it makes sense at all.

EDIT: Created an issue out of this: #1830

@moneymanolis
Copy link
Collaborator

@relativisticelectron regarding 267cb99 initiating addRecipientwith amount = 0 made the Cypress type command basically append X to have an amount 0X in the tests. This made the tests flaky / failing. Also, this made the placeholder of 0 pointless. Just double-checking with you whether you somehow wanted to have the 0.

@relativisticelectron
Copy link
Collaborator Author

@relativisticelectron regarding 267cb99 initiating addRecipientwith amount = 0 made the Cypress type command basically append X to have an amount 0X in the tests. This made the tests flaky / failing. Also, this made the placeholder of 0 pointless. Just double-checking with you whether you somehow wanted to have the 0.

Thanks for fixing that.

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.

UI Bug: "RBF Enabled" gets hidden
4 participants