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

[ANCHOR-380] Add client_id to getRate() in Sep31Service #1068

Merged
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions core/src/main/java/org/stellar/anchor/config/ClientsConfig.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package org.stellar.anchor.config;

import lombok.AllArgsConstructor;
import lombok.Data;
import lombok.NoArgsConstructor;

public interface ClientsConfig {
@Data
@AllArgsConstructor
@NoArgsConstructor
class ClientConfig {
String name;
ClientType type;
String signingKey;
String domain;
String callbackUrl;
}

enum ClientType {
CUSTODIAL,
NONCUSTODIAL
}

ClientConfig getClientConfigBySigningKey(String signingKey);

ClientConfig getClientConfigByDomain(String domain);
}
15 changes: 2 additions & 13 deletions core/src/main/java/org/stellar/anchor/config/Sep10Config.java
Original file line number Diff line number Diff line change
Expand Up @@ -67,23 +67,12 @@ public interface Sep10Config {
boolean isClientAttributionRequired();

/**
* Get the list of allowed client domains if the client attribution is required.
* Get the list of allowed client names if the client attribution is required.
*
* @return the list of allowed client domains.
* @return the list of allowed client names.
*/
List<String> getClientAttributionAllowList();

/**
* Whether to require authenticating clients to be in the list of known custodial accounts. # # If
* the flag is set to true, the client must be one of the custodial clients defined in the clients
* section # of this configuration file.
*
* <p>The flag is only relevant for custodial wallets.
*
* @return true if known custodial account is required.
*/
boolean isKnownCustodialAccountRequired();

/**
* Set the list of known custodial accounts.
*
Expand Down
14 changes: 3 additions & 11 deletions core/src/main/java/org/stellar/anchor/sep10/Sep10Service.java
Original file line number Diff line number Diff line change
Expand Up @@ -76,17 +76,9 @@ public ChallengeResponse createChallenge(ChallengeRequest challengeRequest) thro
sep10Config.getKnownCustodialAccountList().contains(challengeRequest.getAccount().trim());
}

if (sep10Config.isKnownCustodialAccountRequired() && !custodialWallet) {
// validate that requesting account is allowed access
infoF("requesting account: {} is not in allow list", challengeRequest.getAccount().trim());
throw new SepNotAuthorizedException("unable to process");
}

if (custodialWallet) {
if (challengeRequest.getClientDomain() != null) {
throw new SepValidationException(
"client_domain must not be specified if the account is an custodial-wallet account");
}
if (custodialWallet && challengeRequest.getClientDomain() != null) {
throw new SepValidationException(
"client_domain must not be specified if the account is an custodial-wallet account");
}

if (!custodialWallet && sep10Config.isClientAttributionRequired()) {
Expand Down
38 changes: 37 additions & 1 deletion core/src/main/java/org/stellar/anchor/sep31/Sep31Service.java
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@
import org.stellar.anchor.asset.AssetService;
import org.stellar.anchor.auth.Sep10Jwt;
import org.stellar.anchor.config.AppConfig;
import org.stellar.anchor.config.ClientsConfig;
import org.stellar.anchor.config.ClientsConfig.ClientConfig;
import org.stellar.anchor.config.Sep10Config;
import org.stellar.anchor.config.Sep31Config;
import org.stellar.anchor.event.EventService;
import org.stellar.anchor.sep38.Sep38Quote;
Expand All @@ -46,10 +49,12 @@

public class Sep31Service {
private final AppConfig appConfig;
private final Sep10Config sep10Config;
private final Sep31Config sep31Config;
private final Sep31TransactionStore sep31TransactionStore;
private final Sep31DepositInfoGenerator sep31DepositInfoGenerator;
private final Sep38QuoteStore sep38QuoteStore;
private final ClientsConfig clientsConfig;
private final AssetService assetService;
private final FeeIntegration feeIntegration;
private final CustomerIntegration customerIntegration;
Expand All @@ -60,21 +65,25 @@ public class Sep31Service {

public Sep31Service(
AppConfig appConfig,
Sep10Config sep10Config,
Sep31Config sep31Config,
Sep31TransactionStore sep31TransactionStore,
Sep31DepositInfoGenerator sep31DepositInfoGenerator,
Sep38QuoteStore sep38QuoteStore,
ClientsConfig clientsConfig,
AssetService assetService,
FeeIntegration feeIntegration,
CustomerIntegration customerIntegration,
EventService eventService) {
debug("appConfig:", appConfig);
debug("sep31Config:", sep31Config);
this.appConfig = appConfig;
this.sep10Config = sep10Config;
this.sep31Config = sep31Config;
this.sep31TransactionStore = sep31TransactionStore;
this.sep31DepositInfoGenerator = sep31DepositInfoGenerator;
this.sep38QuoteStore = sep38QuoteStore;
this.clientsConfig = clientsConfig;
this.assetService = assetService;
this.feeIntegration = feeIntegration;
this.customerIntegration = customerIntegration;
Expand Down Expand Up @@ -508,13 +517,40 @@ void updateFee() throws SepValidationException, AnchorException {
.receiveAmount(null)
.senderId(request.getSenderId())
.receiverId(request.getReceiverId())
.clientId(token.getAccount())
.clientId(getClientName())
.build())
.getFee();
infoF("Fee for request ({}) is ({})", request, fee);
Context.get().setFee(fee);
}

String getClientName() throws BadRequestException {
return getClientName(Context.get().getSep10Jwt().getAccount());
}

String getClientName(String account) throws BadRequestException {
JiahuiWho marked this conversation as resolved.
Show resolved Hide resolved
ClientConfig client = clientsConfig.getClientConfigBySigningKey(account);
if (!sep10Config.getClientAttributionAllowList().isEmpty()) {
// check if the client is not in the allow list, client should be denied
if (client != null
&& !sep10Config.getClientAttributionAllowList().contains(client.getName())) {
client = null;
}
}

if (sep10Config.isClientAttributionRequired()) {
if (client == null) {
throw new BadRequestException("Client not found");
}
} else {
// client attribution not required
if (client == null) {
return null;
}
}
return client.getName();
}

JiahuiWho marked this conversation as resolved.
Show resolved Hide resolved
/**
* validateSenderAndReceiver will validate if the SEP-31 sender and receiver exist and their
* status is ACCEPTED.
Expand Down
55 changes: 37 additions & 18 deletions core/src/main/java/org/stellar/anchor/sep38/Sep38Service.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@

import static org.stellar.anchor.api.sep.sep38.Sep38Context.SEP31;
import static org.stellar.anchor.api.sep.sep38.Sep38Context.SEP6;
import static org.stellar.anchor.api.sep.sep38.Sep38QuoteResponse.*;
import static org.stellar.anchor.event.EventService.EventQueue.TRANSACTION;
import static org.stellar.anchor.util.BeanHelper.updateField;
import static org.stellar.anchor.util.Log.debug;
import static org.stellar.anchor.util.Log.*;
import static org.stellar.anchor.util.MathHelper.decimal;
import static org.stellar.anchor.util.MathHelper.formatAmount;
import static org.stellar.anchor.util.MetricConstants.SEP38_PRICE_QUERIED;
Expand All @@ -20,9 +21,7 @@
import java.time.Instant;
import java.util.*;
import org.apache.commons.lang3.StringUtils;
import org.stellar.anchor.api.callback.GetRateRequest;
import org.stellar.anchor.api.callback.GetRateResponse;
import org.stellar.anchor.api.callback.RateIntegration;
import org.stellar.anchor.api.callback.*;
import org.stellar.anchor.api.event.AnchorEvent;
import org.stellar.anchor.api.exception.AnchorException;
import org.stellar.anchor.api.exception.BadRequestException;
Expand Down Expand Up @@ -139,7 +138,8 @@ public void validateAsset(String prefix, String assetName) throws AnchorExceptio
}
}

public GetPriceResponse getPrice(Sep38GetPriceRequest getPriceRequest) throws AnchorException {
public GetPriceResponse getPrice(Sep10Jwt token, Sep38GetPriceRequest getPriceRequest)
throws AnchorException {
String sellAssetName = getPriceRequest.getSellAssetName();
String sellAmount = getPriceRequest.getSellAmount();
String sellDeliveryMethod = getPriceRequest.getSellDeliveryMethod();
Expand Down Expand Up @@ -215,7 +215,7 @@ public GetPriceResponse getPrice(Sep38GetPriceRequest getPriceRequest) throws An
validateAmountLimit("sell_", sellAmount, sendMinLimit, sendMaxLimit);
}

GetRateRequest request =
GetRateRequest.GetRateRequestBuilder rrBuilder =
JiahuiWho marked this conversation as resolved.
Show resolved Hide resolved
GetRateRequest.builder()
.type(GetRateRequest.Type.INDICATIVE)
.sellAsset(sellAssetName)
Expand All @@ -224,8 +224,15 @@ public GetPriceResponse getPrice(Sep38GetPriceRequest getPriceRequest) throws An
.buyAsset(buyAssetName)
.buyAmount(buyAmount)
.buyDeliveryMethod(buyDeliveryMethod)
.countryCode(countryCode)
.build();
.countryCode(countryCode);

String clientId = getClientIdFromToken(token);
if (clientId != null) {
rrBuilder.clientId(clientId);
}

// Get the rate
GetRateRequest request = rrBuilder.build();
GetRateResponse rateResponse = this.rateIntegration.getRate(request);
GetRateResponse.Rate rate = rateResponse.getRate();

Expand Down Expand Up @@ -348,7 +355,7 @@ public Sep38QuoteResponse postQuote(Sep10Jwt token, Sep38PostQuoteRequest reques
validateAmountLimit("sell_", request.getSellAmount(), sendMinLimit, sendMaxLimit);
}

GetRateRequest getRateRequest =
GetRateRequest.GetRateRequestBuilder getRateRequestBuilder =
GetRateRequest.builder()
.type(GetRateRequest.Type.FIRM)
.sellAsset(request.getSellAssetName())
Expand All @@ -358,11 +365,17 @@ public Sep38QuoteResponse postQuote(Sep10Jwt token, Sep38PostQuoteRequest reques
.buyAmount(request.getBuyAmount())
.buyDeliveryMethod(request.getBuyDeliveryMethod())
.countryCode(request.getCountryCode())
.expireAfter(request.getExpireAfter())
.clientId(account)
.build();
GetRateResponse.Rate rate = this.rateIntegration.getRate(getRateRequest).getRate();
.expireAfter(request.getExpireAfter());

// Sets the clientId if the customer exists
String clientId = getClientIdFromToken(token);
if (clientId != null) {
getRateRequestBuilder.clientId(clientId);
}
GetRateRequest getRateRequest = getRateRequestBuilder.build();
Comment on lines -361 to +375
Copy link
Contributor

Choose a reason for hiding this comment

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

Firm quotes will always be authenticated, so we don't need to call getClientIdFromToken(). If the token is null, we should throw an error, rather than handle it gracefully.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The check of the authentication with the token is always done at the controller. We don't need to worry about them in the service layer. It makes simpler not to worry about authentication in the service layer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, so if the controller makes sure that the token is present, we don't need to check it clientId is null and can just do

getRateRequestBuilder.clientId(token.getAccount())


// Get and set the rate
GetRateResponse.Rate rate = this.rateIntegration.getRate(getRateRequest).getRate();
if (rate.getBuyAmount() == null || rate.getSellAmount() == null) {
throw new ServerErrorException(
String.format(
Expand All @@ -375,8 +388,8 @@ public Sep38QuoteResponse postQuote(Sep10Jwt token, Sep38PostQuoteRequest reques
decimal(rate.getSellAmount(), pricePrecision),
decimal(rate.getBuyAmount(), pricePrecision));

Sep38QuoteResponse.Sep38QuoteResponseBuilder builder =
Sep38QuoteResponse.builder()
Sep38QuoteResponse.Sep38QuoteResponseBuilder responseBuilder =
builder()
.id(rate.getId())
.expiresAt(rate.getExpiresAt())
.price(rate.getPrice())
Expand All @@ -386,7 +399,8 @@ public Sep38QuoteResponse postQuote(Sep10Jwt token, Sep38PostQuoteRequest reques

String sellAmount = formatAmount(decimal(rate.getSellAmount()), sellAsset.getDecimals());
String buyAmount = formatAmount(decimal(rate.getBuyAmount()), buyAsset.getDecimals());
builder = builder.totalPrice(totalPrice).sellAmount(sellAmount).buyAmount(buyAmount);
responseBuilder =
responseBuilder.totalPrice(totalPrice).sellAmount(sellAmount).buyAmount(buyAmount);

// SEP31: when buy_amount is specified (sell amount found from rate integration)
if (context == SEP31 && isNotEmpty(buyAmount)) {
Expand Down Expand Up @@ -449,7 +463,7 @@ public Sep38QuoteResponse postQuote(Sep10Jwt token, Sep38PostQuoteRequest reques

// increment counter
sep38QuoteCreated.increment();
return builder.build();
return responseBuilder.build();
}

public Sep38QuoteResponse getQuote(Sep10Jwt token, String quoteId) throws AnchorException {
Expand Down Expand Up @@ -488,7 +502,7 @@ public Sep38QuoteResponse getQuote(Sep10Jwt token, String quoteId) throws Anchor
throw new NotFoundException("quote not found");
}

return Sep38QuoteResponse.builder()
return builder()
.id(quote.getId())
.expiresAt(quote.getExpiresAt())
.totalPrice(quote.getTotalPrice())
Expand All @@ -507,4 +521,9 @@ private String getTotalPrice(BigDecimal bSellAmount, BigDecimal bBuyAmount) {

return formatAmount(bTotalPrice, pricePrecision);
}

private String getClientIdFromToken(Sep10Jwt token) {
if (token == null) return null;
return token.getAccount();
}
Comment on lines +525 to +528
Copy link
Contributor

Choose a reason for hiding this comment

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

This works until we add support for using SEP-38 prices & quotes for SEP-24 & SEP-6 transactions. If we want to replace the usage of public keys and domains with client names, we can't merge this because it would be a breaking change to change clientId from a public key to names defined in the config. cc @Ifropc

}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import okhttp3.mockwebserver.MockResponse
import okhttp3.mockwebserver.MockWebServer
import org.junit.jupiter.api.*
import org.junit.jupiter.api.Assertions.assertEquals
import org.junit.jupiter.api.Assertions.assertInstanceOf
import org.junit.jupiter.api.MethodOrderer.OrderAnnotation
import org.junit.jupiter.params.ParameterizedTest
import org.junit.jupiter.params.provider.CsvSource
Expand Down Expand Up @@ -603,8 +602,7 @@ internal class Sep10ServiceTest {
}

@Test
fun `test createChallenge() ok when knownCustodialAccountRequired is enabled`() {
every { sep10Config.isKnownCustodialAccountRequired } returns true
fun `test createChallenge() ok`() {
every { sep10Config.knownCustodialAccountList } returns listOf(TEST_ACCOUNT)
val cr =
ChallengeRequest.builder()
Expand All @@ -615,13 +613,11 @@ internal class Sep10ServiceTest {
.build()

assertDoesNotThrow { sep10Service.createChallenge(cr) }
verify(exactly = 1) { sep10Config.isKnownCustodialAccountRequired }
verify(exactly = 2) { sep10Config.knownCustodialAccountList }
}

@Test
fun `Test createChallenge() when isKnownCustodialAccountRequired is not enabled`() {
JiahuiWho marked this conversation as resolved.
Show resolved Hide resolved
every { sep10Config.isKnownCustodialAccountRequired } returns false
every { sep10Config.knownCustodialAccountList } returns
listOf("G321E23CFSUMSVQK4Y5JHPPYK73VYCNHZHA7ENKCV37P6SUEO6XQBKPP")
val cr =
Expand All @@ -633,27 +629,6 @@ internal class Sep10ServiceTest {
.build()

assertDoesNotThrow { sep10Service.createChallenge(cr) }
verify(exactly = 1) { sep10Config.isKnownCustodialAccountRequired }
verify(exactly = 2) { sep10Config.knownCustodialAccountList }
}

@Test
fun `test createChallenge() failure when isRequireKnownOmnibusAccount is enabled and account mis-match`() {
every { sep10Config.isKnownCustodialAccountRequired } returns true
every { sep10Config.knownCustodialAccountList } returns
listOf("G321E23CFSUMSVQK4Y5JHPPYK73VYCNHZHA7ENKCV37P6SUEO6XQBKPP")
val cr =
ChallengeRequest.builder()
.account(TEST_ACCOUNT)
.memo(TEST_MEMO)
.homeDomain(TEST_HOME_DOMAIN)
.clientDomain(null)
.build()

val ex = assertThrows<SepException> { sep10Service.createChallenge(cr) }
verify(exactly = 1) { sep10Config.isKnownCustodialAccountRequired }
verify(exactly = 2) { sep10Config.knownCustodialAccountList }
assertInstanceOf(SepNotAuthorizedException::class.java, ex)
assertEquals("unable to process", ex.message)
}
}
Loading
Loading