-
Notifications
You must be signed in to change notification settings - Fork 11
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
SEP-38: update SEP-38 tests based on stellar/stellar-protocol#1204 #86
Conversation
…in the stellar network. This is related to stellar/stellar-protocol#1149.
Preview is available here: |
1 similar comment
Preview is available here: |
Preview is available here: |
|
||
const notValidFiat = parts.length === 2 && parts[0] !== "iso4217"; | ||
const notValidStellar = parts.length === 3 && parts[0] !== "stellar"; | ||
const notValidAtAll = parts.length < 2 || parts.length > 3; | ||
if (notValidFiat || notValidStellar || notValidAtAll) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this change because since stellar/stellar-protocol#1149, SEP-31 accepts delivering on-chain access as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a expert in TS. But LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple questions, but everything else looks good to me.
@@ -78,6 +81,7 @@ export const returnsValidResponse: Test = { | |||
sell_asset: this.context.expects.sep38StellarAsset, | |||
buy_asset: this.context.expects.sep38OffChainAsset, | |||
sell_amount: "100", | |||
context: "sep31", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tough, because an anchor could support SEP-38 for SEP-6, and we'd be sending an invalid context value. Maybe we should add a config attribute that allows the user to indicate which context
value should be used? Or maybe we can infer which value to use based on the tests being run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, did we update the config file info displayed in the UI after adding SEP-38 support? Or does SEP-38 not require any config currently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sep38 doesn't require any context at the moment. I'll add the context in the next PR and make it configurable in the sep config.
…e tested (#87) ### What SEP-38 now requires a config file to indicate which contexts should be tested. Also, update some SEP-38 tests to execute the request once for each context. ### Why The SEP-38 `GET /price` and `POST /quote` request parameters have a new mandatory field called `context`, that can be set to "sep6" or "sep31". With the new configuration file, Anchors can choose which context(s) should be used in the tests. Addresses #86 (comment). ### Pending * Update documentation. I'll do that in another PR so it's easier to review.
What
Update SEP-38 tests based on stellar/stellar-protocol#1204. The changes include:
GET /price
andPOST /quote
now require the mandatorycontext
request parameter.GET /price
andGET|POST /quote
now return the mandatory response parameterstotal_price
andfee
.