Nma 1067 custodial swaps sell dash by hadia · Pull Request #926 · dashpay/dash-wallet

hadia · 2022-03-31T13:22:38Z

As a user who has a Coinbase account I would like to be able to swap my dash in my Dash Wallet to another crypto into my Coinbase account.

Issue being fixed or feature implemented

Related PR's and Dependencies

Screenshots / Videos

How Has This Been Tested?

QA (Mobile Team)

Checklist:

I have performed a self-review of my own code and added comments where necessary
I have added or updated relevant unit/integration/functional/e2e tests

…waps_Sell_Dash # Conflicts: # integrations/coinbase-integration/src/main/java/org/dash/wallet/integration/coinbase_integration/ui/CoinbaseBuyDashOrderReviewFragment.kt # integrations/coinbase-integration/src/main/res/values/strings.xml

HashEngineering · 2022-03-31T13:54:11Z

+interface SendPaymentService {
+    suspend fun sendCoins(
+        address: Address,
+        amount: Coin,
+        constrainInputsTo: Address? = null,
+        emptyWallet: Boolean = false
+    ): Transaction
+}


@Syn-McJ Didn't also make a similar service? or is this the same one?

It's the same one as I understand, Hadia has asked me about sending coins and I have referenced my solution.

Syn-McJ

Looks good, but I have some comments/questions. I'm mostly concerned with AlertDialog that is going to stick on top of the lock screen

Syn-McJ · 2022-03-31T13:56:09Z

    }

+    private fun show2FADialog() {
+        val builder: AlertDialog.Builder = android.app.AlertDialog.Builder(requireContext())


AlertDialog will not be dismissed automatically when the lock screen shows up. Can this be implemented with a DialogFragment subclass instead?

For this its just temp code and will be replaced in with the new 2FA Dialog

Syn-McJ · 2022-03-31T13:57:54Z

    }
+
+    private fun show2FADialog() {
+        val builder: AlertDialog.Builder = android.app.AlertDialog.Builder(requireContext())


Same point about AlertDialog (see above)

For this its just temp code and will be replaced in with the new 2FA Dialog

Syn-McJ · 2022-03-31T14:00:15Z

            binding.youWillReceiveValue.isVisible = hasBalance
            if (hasBalance) {
-                binding.youWillReceiveValue.text = context?.getString(R.string.you_will_receive_dash, dashFormat.format(balance).toString())
+                binding.youWillReceiveValue.text = context?.getString(


You can use requireContext here.

Suggested change

binding.youWillReceiveValue.text = context?.getString(

binding.youWillReceiveValue.text = requireContext().getString(

Syn-McJ · 2022-03-31T14:08:31Z

+interface SendPaymentService {
+    suspend fun sendCoins(
+        address: Address,
+        amount: Coin,
+        constrainInputsTo: Address? = null,
+        emptyWallet: Boolean = false
+    ): Transaction
+}


It's the same one as I understand, Hadia has asked me about sending coins and I have referenced my solution.

Syn-McJ · 2022-03-31T14:12:48Z

    }

-    private fun getFaitAmount(balance: String, currencyCode: String): Fiat? {
+    private fun getFaitAmount(balance: String, currencyCode: String): Pair<Fiat?, Coin?>? {


Typo in the name I think

Syn-McJ · 2022-03-31T14:15:53Z

+        viewModel.selectedCryptoCurrencyAccount.value?.let { userAccountData ->
+
+
+            val cleanedValue =


This method (setAmountValue) seems quite convoluted, and it's all just for getting a clean value. Maybe it's worth to think about how to simplify it / extract some parts to separate methods

ClaudeHangui · 2022-03-31T14:43:59Z

    fun setOnSwapDashFromToCryptoClicked(dashToCrypto: Boolean) {
+        if (dashToCrypto) {
+            if (walletDataProvider.getWalletBalance().isZero) {
+                _userDashAccountEmptyError.value = true


since _userDashAccountEmptyError.value = true always applies, I think you're better off using SingleLiveEvent<Unit>

ClaudeHangui · 2022-03-31T14:45:32Z

 import java.math.RoundingMode
 import javax.inject.Inject

 @ExperimentalCoroutinesApi


you might as well add this annotation in the CoinbaseConvertCryptoFragment class

ClaudeHangui · 2022-03-31T14:48:13Z

+    private val walletDataProvider: WalletDataProvider
 ) : ViewModel() {

+    val dashFormat = MonetaryFormat().withLocale(GenericUtils.getDeviceLocale())


This property could be private if used only within this class

ClaudeHangui · 2022-03-31T14:51:24Z

+
    private val _dashToCrypto = MutableLiveData<Boolean>()
    val dashToCrypto: LiveData<Boolean>
        get() = this._dashToCrypto


is there a specific reason for appending this keyword in some of the properties used within this class ?!
I think you can get rid of it

ClaudeHangui · 2022-03-31T15:03:06Z

        _dashToCrypto.value = dashToCrypto
    }

    fun clear() { _selectedCryptoCurrencyAccount.value = null }


Since this function is to be called when the view is no more used, you might as well call it directly in onCleared() function of the VM which is automatically called when fragment is destroyed

ClaudeHangui

Works for me !
slight improvements though could be made in some view models

Built from platform `integration/v41-keystore-qa5` @ 7c495eab76. Three changes over v41int19: * **Pooled asset-lock funding** (dashpay/platform#4350). An invitation, identity registration or top-up now funds from the union of the BIP44 and BIP32 accounts at the given index plus every DashPay contact-receiving account, with change back to BIP44 — the same set the send path has pooled since #4329. A lock no longer needs its whole amount sitting in one account, so the sweep-then-lock hop (an extra on-chain transaction, an extra fee, and a reused transparent address) is gone. Reservation release after a rejected broadcast now reaches every contributing account under the build's owner token, and the funding-transaction lookup that gates the proof wait searches all four account families — a lock funded entirely out of BIP32 or a contact account was previously invisible to it. Shielded funding and CoinJoin funding are unchanged: both still draw from the one account the caller names. Carries the rust-dashcore pin from 8f26456b to d44d241d, which is that commit plus exactly one — dashpay/rust-dashcore#935, the pooled builder this depends on. * **Contact watch-only records no longer define the persisted transaction row** (#926 completion). A payment TO a contact stopped being persisted as incoming, and the contact's own coins stopped entering `txos`. * Pre-existing clippy/rustfmt debt on the qa5 line cleared, so the lint gate is green again. Verified before publishing: platform-wallet 637 Rust tests, platform-wallet-ffi 254, rs-unified-sdk-jni 38, Kotlin SDK 436 unit tests, `cargo clippy --lib --tests -- -D warnings` and `cargo fmt --check` clean, and both JNI libraries verified (241 exports, 16 KB-aligned LOAD segments, arm64-v8a + x86_64). Here: `:wallet:compile_testNet3DebugKotlin` plus the full unit suite — 1705 tests, 0 failures. No APK built and nothing installed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

hadia added 16 commits March 10, 2022 16:18

fix: add dash balance

dc95cbc

feat: add error dialog

3fcdc33

feat: send dash to coinbase

244dc78

feat: send dash to coinbase

589e10b

feat: send dash to coinbase

832ce8a

feat: send dash sync

6d72f37

feat: send dash with 2fa

a49b68a

feat: send dash

e1448bf

feat: update test

b811928

feat: update branch

861f7af

feat: Add error for sending coins

006e3a0

fix: nav direction and fait error

6114627

fix: null pointer for dash input

8797cf8

fix: min value issue

f31eabc

fix: min value issue

e92d795

hadia requested review from ClaudeHangui, HashEngineering and Syn-McJ March 31, 2022 13:22

HashEngineering reviewed Mar 31, 2022

View reviewed changes

Syn-McJ reviewed Mar 31, 2022

View reviewed changes

ClaudeHangui reviewed Mar 31, 2022

View reviewed changes

Syn-McJ approved these changes Mar 31, 2022

View reviewed changes

ClaudeHangui approved these changes Mar 31, 2022

View reviewed changes

HashEngineering merged commit 8cb7505 into feature-coinbase-integration Apr 1, 2022

HashEngineering deleted the NMA-1067_Custodial_Swaps_Sell_Dash branch July 20, 2022 23:20

	binding.youWillReceiveValue.text = context?.getString(
	binding.youWillReceiveValue.text = requireContext().getString(

		viewModel.selectedCryptoCurrencyAccount.value?.let { userAccountData ->


		val cleanedValue =

Uh oh!

Conversation

hadia commented Mar 31, 2022

Issue being fixed or feature implemented

Related PR's and Dependencies

Screenshots / Videos

How Has This Been Tested?

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Syn-McJ Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Syn-McJ left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Syn-McJ Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ClaudeHangui left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Syn-McJ Mar 31, 2022 •

edited

Loading

Syn-McJ Mar 31, 2022 •

edited

Loading