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

DEX create password, DCR wallet and add server #676

Merged
merged 17 commits into from Dec 21, 2021

Conversation

song50119
Copy link
Contributor

@song50119 song50119 commented Oct 21, 2021

Work towards #619

  • Initialize app password
  • Add wallet for dcr to use to pay the dex server fee
  • Add dex server this come before adding any wallet
  • Show the fee confirmations

Test server: dex-test.ssgen.io:7232

Copy link
Contributor

@itswisdomagain itswisdomagain left a comment

Choose a reason for hiding this comment

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

Looks good and works well. Major change required is connecting a DEX before connecting a wallet to pay fees. Also, if there are no plans to add assets aside dcr and btc for now, no need to initialize them or add their icons.

While reviewing, I tinkered with some of the requested changes and made a commit here for your perusal: itswisdomagain@5bcef82

ui/page/main_page.go Outdated Show resolved Hide resolved
ui/page/dexclient/markets.go Outdated Show resolved Hide resolved
ui/page/dexclient/markets.go Outdated Show resolved Hide resolved
ui/page/dexclient/markets.go Outdated Show resolved Hide resolved
ui/page/dexclient/markets.go Outdated Show resolved Hide resolved
ui/page/dexclient/listeners.go Outdated Show resolved Hide resolved
ui/page/dexclient/listeners.go Outdated Show resolved Hide resolved
ui/page/dexclient/login_modal.go Outdated Show resolved Hide resolved
ui/page/dexclient/password_modal.go Outdated Show resolved Hide resolved
ui/page/dexclient/unlock_wallet_modal.go Outdated Show resolved Hide resolved
@song50119 song50119 force-pushed the dex-integrate-1 branch 3 times, most recently from 4af88ba to ec23c9a Compare October 27, 2021 13:47
Copy link
Contributor

@itswisdomagain itswisdomagain left a comment

Choose a reason for hiding this comment

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

Almost there. Just a few nits from me now.

dexc/dexc.go Outdated Show resolved Hide resolved
dexc/dexc.go Outdated Show resolved Hide resolved
dexc/log.go Outdated Show resolved Hide resolved
log.go Outdated Show resolved Hide resolved
ui/page/main_page.go Outdated Show resolved Hide resolved
ui/page/dexclient/login_modal.go Outdated Show resolved Hide resolved
ui/page/dexclient/add_dex_modal.go Outdated Show resolved Hide resolved
ui/page/dexclient/create_wallet_modal.go Outdated Show resolved Hide resolved
Copy link
Contributor

@itswisdomagain itswisdomagain left a comment

Choose a reason for hiding this comment

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

Looks good to go from my end. Just one more possible improvement.

ui/page/dexclient/confirm_register_modal.go Outdated Show resolved Hide resolved
@itswisdomagain
Copy link
Contributor

itswisdomagain commented Oct 29, 2021

I have this issue with fee payments sometimes not getting broadcasted until godcr restart. Occasionally, I see the following error logs after paying the dex fee but the fee tx may still not be broadcasted even if there is no error log:

2021-10-29 09:33:43.025 [INF] DLWL: Inserting unconfirmed transaction b90cdf32172c7e699be17ac9728d77671dbbd33cdaf58f515c315063f772e751
2021-10-29 09:33:43.105 [INF] DLWL: [1] New Transaction b90cdf32172c7e699be17ac9728d77671dbbd33cdaf58f515c315063f772e751
2021-10-29 08:33:43.267 [INF] CORE: notify: |SUCCESS| (feepayment) Fee payment in progress - Waiting for 1 confirmations before trading at dex-test.ssgen.io:7232
2021-10-29 09:33:43.827 [WRN] DLWL: 47.242.243.123:19108 reject(tx, REJECT_INSUFFICIENTFEE, b90cdf32172c7e699be17ac9728d77671dbbd33cdaf58f515c315063f772e751): transaction b90cdf32172c7e699be17ac9728d77671dbbd33cdaf58f515c315063f772e751 has insufficient priority (6.32261967816092e+06 <= 5.76e+07)
2021-10-29 09:33:43.925 [WRN] DLWL: 116.62.229.19:19108 reject(tx, REJECT_INSUFFICIENTFEE, b90cdf32172c7e699be17ac9728d77671dbbd33cdaf58f515c315063f772e751): transaction b90cdf32172c7e699be17ac9728d77671dbbd33cdaf58f515c315063f772e751 has insufficient priority (6.32261967816092e+06 <= 5.76e+07)

In all cases though, I see

2021-10-29 09:33:43.025 [INF] DLWL: Inserting unconfirmed transaction b90cdf32172c7e699be17ac9728d77671dbbd33cdaf58f515c315063f772e751
2021-10-29 09:33:43.105 [INF] DLWL: [1] New Transaction b90cdf32172c7e699be17ac9728d77671dbbd33cdaf58f515c315063f772e751

but the tx may not show up on dcrdata until godcr is restarted.

My guess is the dex is using a too little miners fee for the dex fee tx which causes it to be rejected by spv peers until godcr restart where dcrlibwallet/dcrwallet resends pending transactions. Confirming this theory will require constructing and sending raw transactions with small fees using dcrlibwallet, to see if the sent transaction gets indexed by dcrlibwallet but isn't found on dcrdata.

UPDATE:
A couple godcr restarts later, fee tx isn't on dcrdata still. Reset and re-connected the dex a couple times, same issue, restarting godcr that had fixed this for me isn't working now. I think this is the issue you had one time @song50119, where your dex fee remained at 0 confs for quite some time.

Copy link
Contributor

@itswisdomagain itswisdomagain left a comment

Choose a reason for hiding this comment

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

Just one more change requirement from me. I had tried to test what happens after the fee is confirmed but was unable to because multiple fee payment attempts had their transactions rejected and thus never confirmed. I've tested it now and it doesn't work quite well.

One additional thing, once any modal is displayed on the dex page, the rest of the app cannot be used until the user completes what's required by the modal. For a new dex user, that would require initializing the app, adding a DEX, adding a wallet and paying the fee before the modals disappear and the user can use the rest of the godcr app. Consider adding a close [x] button to each modal or allowing the modals to be closed by clicking outside them.

ui/page/dexclient/markets.go Outdated Show resolved Hide resolved
itswisdomagain
itswisdomagain previously approved these changes Nov 3, 2021
Copy link
Contributor

@Sirmorrison Sirmorrison left a comment

Choose a reason for hiding this comment

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

I believe we need more detailed description on this PR... Reason is i see a few unrelated changes to the issue addressed here.

Like appCTX dex initializing a db on godcr etc.

Also, details on how this PR can be properly tested.

dexc/dexc.go Outdated Show resolved Hide resolved
ui/page/dexclient/add_dex_modal.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
ui/page/dexclient/confirm_register_modal.go Outdated Show resolved Hide resolved
ui/page/dexclient/password_modal.go Outdated Show resolved Hide resolved
@itswisdomagain
Copy link
Contributor

i see a few unrelated changes to the issue addressed here.
Like appCTX dex initializing a db on godcr etc.

Both those changes re appCTX and additional db are part of the DEX client initialization process. I think that initial setup is missing in the PR description, although it might also be too granular to include.

This PR uses a couple dcrdex packages to provide DEX client functionality. The primary package for this purpose is decred.org/dcrdex/client/core which provides methods for initializing the client, adding wallets, placing and canceling orders and executing trades. Once decred.org/dcrdex/client/core is started, a number of background processes may run simultaneously. When starting core, a ctx is required that will be used to signal to core when all processes should shutdown. This context cannot be a random context.Background() but a context that actually gets canceled when the GoDCR app is shutting down; this signals core to also shutdown its activities. That's why the appCtx was introduced. In all honesty though, there should have been an appCtx running around somewhere to ensure pages are closed, background processes stopped when the godcr app is shut down; instead of using context.Background() as I've seen in a couple portions of this codebase.

As explained in a comment above, core needs to maintain its own database to function effectively. The DEX client is a pretty big app on its own and most of the work being done on the GoDCR end will be providing the interface to perform DEX functionality using core behind the scene.

@Sirmorrison
Copy link
Contributor

i see a few unrelated changes to the issue addressed here.
Like appCTX dex initializing a db on godcr etc.

Both those changes re appCTX and additional db are part of the DEX client initialization process. I think that initial setup is missing in the PR description, although it might also be too granular to include.

This PR uses a couple dcrdex packages to provide DEX client functionality. The primary package for this purpose is decred.org/dcrdex/client/core which provides methods for initializing the client, adding wallets, placing and canceling orders and executing trades. Once decred.org/dcrdex/client/core is started, a number of background processes may run simultaneously. When starting core, a ctx is required that will be used to signal to core when all processes should shutdown. This context cannot be a random context.Background() but a context that actually gets canceled when the GoDCR app is shutting down; this signals core to also shutdown its activities. That's why the appCtx was introduced. In all honesty though, there should have been an appCtx running around somewhere to ensure pages are closed, background processes stopped when the godcr app is shut down; instead of using context.Background() as I've seen in a couple portions of this codebase.

As explained in a comment above, core needs to maintain its own database to function effectively. The DEX client is a pretty big app on its own and most of the work being done on the GoDCR end will be providing the interface to perform DEX functionality using core behind the scene.

Valid point.. however, i believe dcrlibwallet is designed for this. We can pass any required parameter to the calling function from the lib.

dcrwallet, VSPD etc operate almost like dex.

Copy link
Contributor

@Sirmorrison Sirmorrison left a comment

Choose a reason for hiding this comment

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

The dex button is always active... You need to use the inactive icon until the dex tab is selected.
image

ui/page/dexclient/password_modal.go Outdated Show resolved Hide resolved
ui/page/dexclient/password_modal.go Outdated Show resolved Hide resolved
ui/load/load.go Show resolved Hide resolved
ui/page/debug_page.go Show resolved Hide resolved
ui/page/dexclient/add_dex_modal.go Outdated Show resolved Hide resolved
ui/page/dexclient/confirm_register_modal.go Outdated Show resolved Hide resolved
ui/page/dexclient/confirm_register_modal.go Outdated Show resolved Hide resolved
ui/page/dexclient/listeners.go Outdated Show resolved Hide resolved
ui/page/dexclient/login_modal.go Outdated Show resolved Hide resolved
ui/page/dexclient/markets.go Outdated Show resolved Hide resolved
@song50119 song50119 force-pushed the dex-integrate-1 branch 2 times, most recently from d9625cf to 9c649cc Compare November 22, 2021 16:14
@song50119 song50119 changed the title Initialize client app and adding the dex server DEX create password, DCR wallet and add server Nov 23, 2021
Copy link
Contributor

@Sirmorrison Sirmorrison left a comment

Choose a reason for hiding this comment

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

image

  • When the Dex page is selected, the button is still the inactive button. not Active. (I have asked the UI/UX guys to get us the logo).
  • Also, When the dex page is opened, is shows password form. My recommendation will be to have a dex welcome describing what dex is about, and then a button for customer proceed. When the button is pressed, the create password modal is shown and the set up process follow. if the modal cancel button is pressed, we remain on the dex welcome page.

This just a proof of concept, however, if this is merged, it will be part of the next prerelease and as such, should look presentable on the app.

ui/page/components/dex.go Outdated Show resolved Hide resolved
@Sirmorrison
Copy link
Contributor

godcr board automation moved this from Review in progress to Approved Dec 10, 2021
@monsa00 monsa00 added this to the 1.0 milestone Dec 10, 2021
@Sirmorrison Sirmorrison removed this from the 1.0 milestone Dec 15, 2021
Copy link
Contributor

@itswisdomagain itswisdomagain left a comment

Choose a reason for hiding this comment

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

While we're here, a minor improvement.

ui/page/dexclient/markets.go Outdated Show resolved Hide resolved
godcr board automation moved this from Approved to Review in progress Dec 21, 2021
song50119 and others added 17 commits December 21, 2021 22:44
- Show the initialize app password modal
- Show the add wallet modal for dcr to use to pay the dex server fee
- Show the add dex server modal this come before adding any wallet
- Show the fee confirmations
- Remove redundant field at loginModal and walletInfoWidget
- Remove Confirm Register Modal
- Change Create Wallet modal to page
- Update menu icon
- Remove add dex page, create wallets page. Use it as widgets
- Remove AppCtx from load and window
- Only create dexCreateWalletWidget when DexClient do not have any wallet
- Add cancel button to Add Dex Modal and Create Wallet Modal
- When app initialize or login check if client is not have dex registered, will go to register new one
- Button "Start sync to continue" will show at the Welcome Page allow to connect to DCR network before use Dex client
- Remove LTC, BCH icons
godcr board automation moved this from Review in progress to Approved Dec 21, 2021
@Sirmorrison Sirmorrison merged commit c4af9c0 into planetdecred:master Dec 21, 2021
godcr board automation moved this from Approved to Done Dec 21, 2021
song50119 added a commit to song50119/godcr that referenced this pull request Apr 24, 2022
* Initialize client app and adding the dex server
- Show the initialize app password modal
- Show the add wallet modal for dcr to use to pay the dex server fee
- Show the add dex server modal this come before adding any wallet
- Show the fee confirmations

* update dcrdex and dcrlibwallet mods

* refactor main page navigation and dex client set up flow

- Remove redundant field at loginModal and walletInfoWidget

* show correct lot size for dex markets

* Fix lint errors

* Update dcrlibwallet version

* use dex client features from dcrlibwallet

* Update dex icon DrawerNav

* Change modals Login, Set App password to page
- Remove Confirm Register Modal
- Change Create Wallet modal to page

* Add welcome page, update login and password modal
- Update menu icon
- Remove add dex page, create wallets page. Use it as widgets

* Only create addDexWidget widget when necessary
- Remove AppCtx from load and window
- Only create dexCreateWalletWidget when DexClient do not have any wallet

* Update dcrlibwallet version

* Add "Add a dex" button to welcome page
- Add cancel button to Add Dex Modal and Create Wallet Modal
- When app initialize or login check if client is not have dex registered, will go to register new one

* Show registration fee payment completed

* Add sync button for Welcome Page
- Button "Start sync to continue" will show at the Welcome Page allow to connect to DCR network before use Dex client
- Remove LTC, BCH icons

* Remove unnecessary field isConnectedToDcrNetwork

Co-authored-by: song50119 <>
Co-authored-by: Wisdom Arerosuoghene <wisdom.arerosuoghene@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
godcr board
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants