Skip to content

Commit

Permalink
Review of API code and added some suggestions for code change and
Browse files Browse the repository at this point in the history 
comments. More details is easier in a direct conversation...
  • Loading branch information
@chimp1984
chimp1984 committed Sep 9, 2020
1 parent 91d70ad commit 961703e
Show file tree
Hide file tree
Showing 15 changed files with 328 additions and 97 deletions.
40 changes: 30 additions & 10 deletions cli/src/main/java/bisq/cli/CliMain.java
View file
Expand Up @@ -58,6 +58,14 @@
@Slf4j
public class CliMain {

/*
* TODO enums follow code style of constants
* "Because they are constants, the names of an enum type's fields are in uppercase letters."
* https://docs.oracle.com/javase/tutorial/java/javaOO/enum.html
*
* I see that you map the cli commands to the enum and stuyle of cli is all lowercase but I think we should use a
* mapper to get less dependency on cli-world with java-world.
*/
private enum Method {
getoffers,
createpaymentacct,
Expand All @@ -72,6 +80,13 @@ private enum Method {
setwalletpassword
}

private static Method getMethodFromCmd(String methodName) {
// TODO if we use const type for enum we need add some mapping
// Even if we don't change now it is handy to have flexibility in case we change internal code and don't want
// to break user commands
return Method.valueOf(methodName.toLowerCase());
}

public static void main(String[] args) {
try {
run(args);
Expand Down Expand Up @@ -114,9 +129,9 @@ public static void run(String[] args) {
}

var methodName = nonOptionArgs.get(0);
final Method method;
Method method;
try {
method = Method.valueOf(methodName);
method = getMethodFromCmd(methodName);
} catch (IllegalArgumentException ex) {
throw new IllegalArgumentException(format("'%s' is not a supported method", methodName));
}
Expand Down Expand Up @@ -149,7 +164,7 @@ public static void run(String[] args) {
return;
}
case getaddressbalance: {
if (nonOptionArgs.size() < 2)
if (nonOptionArgs.size() < 2) // TODO should the args check be more strict with '!=2' ?
throw new IllegalArgumentException("no address specified");

var request = GetAddressBalanceRequest.newBuilder()
Expand All @@ -169,29 +184,34 @@ public static void run(String[] args) {
throw new IllegalArgumentException("incorrect parameter count, expecting direction (buy|sell), currency code");

var direction = nonOptionArgs.get(1);
var fiatCurrency = nonOptionArgs.get(2);
var currencyCode = nonOptionArgs.get(2);

var request = GetOffersRequest.newBuilder()
.setDirection(direction)
.setFiatCurrencyCode(fiatCurrency)
.setCurrencyCode(currencyCode)
.build();
var reply = offersService.getOffers(request);
out.println(formatOfferTable(reply.getOffersList(), fiatCurrency));
out.println(formatOfferTable(reply.getOffersList(), currencyCode));
return;
}
case createpaymentacct: {
/*
* Will require much more work to support diff. account types...
*/
if (nonOptionArgs.size() < 4)
throw new IllegalArgumentException(
"incorrect parameter count, expecting account name, account number, currency code");

var accountName = nonOptionArgs.get(1);
var accountNumber = nonOptionArgs.get(2);
var fiatCurrencyCode = nonOptionArgs.get(3);
var paymentMethodId = nonOptionArgs.get(1).toUpperCase();
var accountName = nonOptionArgs.get(2);
var accountNumber = nonOptionArgs.get(3);
var currencyCode = nonOptionArgs.get(4).toUpperCase();

var request = CreatePaymentAccountRequest.newBuilder()
.setPaymentMethodId(paymentMethodId)
.setAccountName(accountName)
.setAccountNumber(accountNumber)
.setFiatCurrencyCode(fiatCurrencyCode).build();
.setCurrencyCode(currencyCode).build();
paymentAccountsService.createPaymentAccount(request);
out.println(format("payment account %s saved", accountName));
return;
Expand Down
7 changes: 5 additions & 2 deletions core/src/main/java/bisq/core/api/CoreApi.java
View file
Expand Up @@ -127,8 +127,11 @@ public void createOffer(String offerId,
// PaymentAccounts
///////////////////////////////////////////////////////////////////////////////////////////

public void createPaymentAccount(String accountName, String accountNumber, String fiatCurrencyCode) {
paymentAccountsService.createPaymentAccount(accountName, accountNumber, fiatCurrencyCode);
public void createPaymentAccount(String paymentMethodId,
String accountName,
String accountNumber,
String currencyCode) {
paymentAccountsService.createPaymentAccount(paymentMethodId, accountName, accountNumber, currencyCode);
}

public Set<PaymentAccount> getPaymentAccounts() {
Expand Down
73 changes: 62 additions & 11 deletions core/src/main/java/bisq/core/api/CorePaymentAccountsService.java
View file
Expand Up @@ -33,6 +33,8 @@

import lombok.extern.slf4j.Slf4j;

import static com.google.common.base.Preconditions.checkNotNull;

@Slf4j
class CorePaymentAccountsService {

Expand All @@ -49,17 +51,11 @@ public CorePaymentAccountsService(Config config,
this.user = user;
}

public void createPaymentAccount(String accountName, String accountNumber, String fiatCurrencyCode) {
// Create and persist a PerfectMoney dummy payment account. There is no guard
// against creating accounts with duplicate names & numbers, only the uuid and
// creation date are unique.
PaymentMethod dummyPaymentMethod = PaymentMethod.getDummyPaymentMethod(PaymentMethod.PERFECT_MONEY_ID);
PaymentAccount paymentAccount = PaymentAccountFactory.getPaymentAccount(dummyPaymentMethod);
paymentAccount.init();
paymentAccount.setAccountName(accountName);
((PerfectMoneyAccount) paymentAccount).setAccountNr(accountNumber);
paymentAccount.setSingleTradeCurrency(new FiatCurrency(fiatCurrencyCode.toUpperCase()));
user.addPaymentAccount(paymentAccount);
void createPaymentAccount(String paymentMethodId, String accountName, String accountNumber, String currencyCode) {
PaymentAccount paymentAccount = getNewPaymentAccount(paymentMethodId, accountName, accountNumber, currencyCode);

//TODO not sure if there is more to do at account creation. Need to check all the flow when its created from UI
user.addPaymentAccountIfNotExists(paymentAccount);

// Don't do this on mainnet until thoroughly tested.
if (config.baseCurrencyNetwork.isRegtest())
Expand All @@ -68,6 +64,61 @@ public void createPaymentAccount(String accountName, String accountNumber, Strin
log.info("Payment account {} saved", paymentAccount.getId());
}

private PaymentAccount getNewPaymentAccount(String paymentMethodId,
String accountName,
String accountNumber,
String currencyCode) {
PaymentAccount paymentAccount = null;
PaymentMethod paymentMethod = PaymentMethod.getPaymentMethodById(paymentMethodId);
switch (paymentMethod.getId()) {
case PaymentMethod.UPHOLD_ID:
case PaymentMethod.MONEY_BEAM_ID:
case PaymentMethod.POPMONEY_ID:
case PaymentMethod.REVOLUT_ID:
log.error("PaymentMethod {} not supported yet: ", paymentMethod);
break;
case PaymentMethod.PERFECT_MONEY_ID:
// Create and persist a PerfectMoney dummy payment account. There is no guard
// against creating accounts with duplicate names & numbers, only the uuid and
// creation date are unique.

paymentAccount = PaymentAccountFactory.getPaymentAccount(paymentMethod);
paymentAccount.init();
paymentAccount.setAccountName(accountName);
((PerfectMoneyAccount) paymentAccount).setAccountNr(accountNumber);
paymentAccount.setSingleTradeCurrency(new FiatCurrency(currencyCode));
break;
case PaymentMethod.SEPA_ID:
case PaymentMethod.SEPA_INSTANT_ID:
case PaymentMethod.FASTER_PAYMENTS_ID:
case PaymentMethod.NATIONAL_BANK_ID:
case PaymentMethod.SAME_BANK_ID:
case PaymentMethod.SPECIFIC_BANKS_ID:
case PaymentMethod.JAPAN_BANK_ID:
case PaymentMethod.ALI_PAY_ID:
case PaymentMethod.WECHAT_PAY_ID:
case PaymentMethod.SWISH_ID:
case PaymentMethod.CLEAR_X_CHANGE_ID:
case PaymentMethod.CHASE_QUICK_PAY_ID:
case PaymentMethod.INTERAC_E_TRANSFER_ID:
case PaymentMethod.US_POSTAL_MONEY_ORDER_ID:
case PaymentMethod.MONEY_GRAM_ID:
case PaymentMethod.WESTERN_UNION_ID:
case PaymentMethod.CASH_DEPOSIT_ID:
case PaymentMethod.HAL_CASH_ID:
case PaymentMethod.F2F_ID:
case PaymentMethod.PROMPT_PAY_ID:
case PaymentMethod.ADVANCED_CASH_ID:
default:
log.error("PaymentMethod {} not supported yet: ", paymentMethod);
break;
}

checkNotNull(paymentAccount,
"Could not create payment account with paymentMethodId " + paymentMethodId);
return paymentAccount;
}

public Set<PaymentAccount> getPaymentAccounts() {
return user.getPaymentAccounts();
}
Expand Down
52 changes: 32 additions & 20 deletions core/src/main/java/bisq/core/api/CoreWalletsService.java
View file
Expand Up @@ -49,6 +49,10 @@
import static java.lang.String.format;
import static java.util.concurrent.TimeUnit.SECONDS;

// I have the feeling we are doing too much here and will end up with a different impl as it is done in
// BtcWalletService. There is a lot of complexity (and old code). I don't feel confident to review that without much
// more time and would prefer that we take the approach to refactor the existing core classes to be more usable for the
// needs of the API instead adding domain logic here.
@Slf4j
class CoreWalletsService {

Expand All @@ -71,7 +75,7 @@ public CoreWalletsService(Balances balances,
this.btcWalletService = btcWalletService;
}

public long getAvailableBalance() {
long getAvailableBalance() {
verifyWalletsAreAvailable();
verifyEncryptedWalletIsUnlocked();

Expand All @@ -82,40 +86,39 @@ public long getAvailableBalance() {
return balance.getValue();
}

public long getAddressBalance(String addressString) {
Address address = getAddressEntry(addressString).getAddress();
long getAddressBalance(String addressString) {
Address address = btcWalletService.getAddress(addressString);
return btcWalletService.getBalanceForAddress(address).value;
}

public AddressBalanceInfo getAddressBalanceInfo(String addressString) {
AddressBalanceInfo getAddressBalanceInfo(String addressString) {
var satoshiBalance = getAddressBalance(addressString);
var numConfirmations = getNumConfirmationsForMostRecentTransaction(addressString);
return new AddressBalanceInfo(addressString, satoshiBalance, numConfirmations);
}

public List<AddressBalanceInfo> getFundingAddresses() {
List<AddressBalanceInfo> getFundingAddresses() {
verifyWalletsAreAvailable();
verifyEncryptedWalletIsUnlocked();

// Create a new funding address if none exists.
if (btcWalletService.getAvailableAddressEntries().size() == 0)
if (btcWalletService.getAvailableAddressEntries().isEmpty()) {
btcWalletService.getFreshAddressEntry();
}

List<String> addressStrings =
btcWalletService
.getAvailableAddressEntries()
.stream()
.map(AddressEntry::getAddressString)
.collect(Collectors.toList());
List<String> addressStrings = btcWalletService
.getAvailableAddressEntries()
.stream()
.map(AddressEntry::getAddressString)
.collect(Collectors.toList());

// getAddressBalance is memoized, because we'll map it over addresses twice.
// To get the balances, we'll be using .getUnchecked, because we know that
// this::getAddressBalance cannot return null.
var balances = memoize(this::getAddressBalance);

boolean noAddressHasZeroBalance =
addressStrings.stream()
.allMatch(addressString -> balances.getUnchecked(addressString) != 0);
boolean noAddressHasZeroBalance = addressStrings.stream()
.allMatch(addressString -> balances.getUnchecked(addressString) != 0);

if (noAddressHasZeroBalance) {
var newZeroBalanceAddress = btcWalletService.getFreshAddressEntry();
Expand All @@ -129,13 +132,13 @@ public List<AddressBalanceInfo> getFundingAddresses() {
.collect(Collectors.toList());
}

public int getNumConfirmationsForMostRecentTransaction(String addressString) {
int getNumConfirmationsForMostRecentTransaction(String addressString) {
Address address = getAddressEntry(addressString).getAddress();
TransactionConfidence confidence = btcWalletService.getConfidenceForAddress(address);
return confidence == null ? 0 : confidence.getDepthInBlocks();
}

public void setWalletPassword(String password, String newPassword) {
void setWalletPassword(String password, String newPassword) {
verifyWalletsAreAvailable();

KeyCrypterScrypt keyCrypterScrypt = getKeyCrypterScrypt();
Expand Down Expand Up @@ -165,7 +168,9 @@ public void setWalletPassword(String password, String newPassword) {
walletsManager.backupWallets();
}

public void lockWallet() {
//TODO we should stick with the existing domain language we use in the app (e.g. encrypt wallet)
// Otherwise reviewers need to learn the new language and map it to the existing.
void lockWallet() {
if (!walletsManager.areWalletsEncrypted())
throw new IllegalStateException("wallet is not encrypted with a password");

Expand All @@ -175,7 +180,9 @@ public void lockWallet() {
tempAesKey = null;
}

public void unlockWallet(String password, long timeout) {
// I think we duplicate too much domain logic here. Lets move the code where it is used in the UI to the domain
// class where it should be (walletsManager) and re-use that.
void unlockWallet(String password, long timeout) {
verifyWalletIsAvailableAndEncrypted();

KeyCrypterScrypt keyCrypterScrypt = getKeyCrypterScrypt();
Expand All @@ -196,6 +203,11 @@ public void unlockWallet(String password, long timeout) {
lockTask = null;
}

// TODO This adds new not existing and problematic functionality. If a user has open offers he need the key in case
// a taker takes the offer. If the timeout has removed the key take offer fails.
// As we are in the core app domain now we should use existing framework for timers (UserThread.runAfter)
// We need to take care that we do not introduce threading issues. The UserThread.setExecutor() was set in the
// main app.
lockTask = new TimerTask() {
@Override
public void run() {
Expand All @@ -213,7 +225,7 @@ public void run() {

// Provided for automated wallet protection method testing, despite the
// security risks exposed by providing users the ability to decrypt their wallets.
public void removeWalletPassword(String password) {
void removeWalletPassword(String password) {
verifyWalletIsAvailableAndEncrypted();
KeyCrypterScrypt keyCrypterScrypt = getKeyCrypterScrypt();

Expand Down
4 changes: 4 additions & 0 deletions core/src/main/java/bisq/core/btc/wallet/WalletService.java
View file
Expand Up @@ -734,6 +734,10 @@ public static Transaction maybeAddTxToWallet(Transaction transaction,
return maybeAddTxToWallet(transaction.bitcoinSerialize(), wallet, source);
}

public Address getAddress(String addressString) {
return Address.fromBase58(params, addressString);
}


///////////////////////////////////////////////////////////////////////////////////////////
// bisqWalletEventListener
Expand Down
10 changes: 10 additions & 0 deletions core/src/main/java/bisq/core/user/User.java
View file
Expand Up @@ -189,6 +189,16 @@ public boolean hasPaymentAccountForCurrency(TradeCurrency tradeCurrency) {
// Collection operations
///////////////////////////////////////////////////////////////////////////////////////////

public void addPaymentAccountIfNotExists(PaymentAccount paymentAccount) {
if (paymentAccountNotInList(paymentAccount)) {
addPaymentAccount(paymentAccount);
}
}

private boolean paymentAccountNotInList(PaymentAccount paymentAccount) {
return getPaymentAccountsAsObservable().stream().noneMatch(e -> e.equals(paymentAccount));
}

public void addPaymentAccount(PaymentAccount paymentAccount) {
paymentAccount.onAddToUser();

Expand Down

0 comments on commit 961703e

Please sign in to comment.