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

feat(dashdirect): polling API after a puchase #1101

Merged
merged 107 commits into from
Mar 27, 2023

Conversation

Syn-McJ
Copy link
Member

@Syn-McJ Syn-McJ commented Mar 24, 2023

Issue being fixed or feature implemented

  • Instead of waiting 2 seconds after gift card purchase and showing a retry dialog in case info isn't yet available, we will poll the API until we get the info, error or the user dismisses the dialog.
  • BIP70 flow in the SendCoinsTaskRunner is refactored to make it less convoluted.
  • Methods to access PackageInfo-related information moved from WalletApplication into a separate class.
  • Some unused code is removed
  • Some unused resources are removed

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

hadia and others added 30 commits February 20, 2023 23:10
@Syn-McJ Syn-McJ self-assigned this Mar 24, 2023
deviceID = UUID.randomUUID().toString(),
amountUSD = amountValue.toBigDecimal().toDouble(),
paymentCurrency = Constants.DASH_CURRENCY,
deviceID = UUID.randomUUID().toString(), // TODO
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix this in the next story.

val giftCard = GiftCard(
id = giftCardId,
id = UUID.randomUUID().toString(),
Copy link
Member Author

Choose a reason for hiding this comment

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

Because gift card ID isn't available at the time we save the dummy record, I figure it's better to have a UUID here and separate fields for the service.

android:fragment="de.schildbach.wallet.ui.preference.AboutFragment"
android:title="@string/about_title" />

</preference-headers>
Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't look like these fragments are used now anywhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is from the version 6 settings screens.

public String httpUserAgent() {
return httpUserAgent(packageInfo().versionName);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

In the ongoing effort to remove dependencies on the WalletApplication, I moved this code into a separate service class.

@@ -130,102 +122,6 @@ protected void error(Exception x, final int messageResId, final Object... messag
}
}

public final static class BluetoothRequestTask extends RequestPaymentRequestTask {
Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't seem to be used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Bluetooth features were part of the app in prior versions, though they were probably not used by many. Like NFC it was an alternate way to send payments or payment requests to another user's device.

import org.slf4j.LoggerFactory
import java.io.IOException

abstract class HttpDirectPaymentTask(
Copy link
Member Author

Choose a reason for hiding this comment

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

These classes are just Kotlin copies of the java classes and as such not very useful. When we rewrite code to Kotlin we should think about how to make use of coroutines and simplify execution flow for the caller.

val localizedMessage: ResourceString
) : Exception(innerException)

object PaymentIntentParser {
Copy link
Member Author

Choose a reason for hiding this comment

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

InputParser is hard to use from Kotlin and is responsible for too many things. Most callers know which parsing functionality they need and do not require all of the callbacks InputParser has.

I think it's better to have several dedicated parsing classes instead.

Comment on lines -29 to -36
public abstract class ResolveDnsTask {
private final Handler backgroundHandler;
private final Handler callbackHandler;

public ResolveDnsTask(final Handler backgroundHandler) {
this.backgroundHandler = backgroundHandler;
this.callbackHandler = new Handler(Looper.myLooper());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may have been used for the Trusted Peer feature which didn't make from version 6 to version 7.

It is a feature that I would like to bring back, but we can always go back in time to get the code and probably rewrite most of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. If there is a story for bringing it back, we can mention this file in there.

Copy link
Collaborator

@HashEngineering HashEngineering left a comment

Choose a reason for hiding this comment

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

Looks great.

@HashEngineering HashEngineering merged commit 5ea7c04 into feature-dashdirect Mar 27, 2023
@Syn-McJ Syn-McJ deleted the feature-dashdirect-polling branch April 10, 2023 08:07
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.

None yet

3 participants