Let SEP-31 POST transaction depend on SEP-38 POST quote #90
Conversation
|
Preview is available here: |
| @@ -55,13 +56,15 @@ const canCreateTransaction: Test = { | |||
| hasDirectPaymentServer, | |||
| hasExpectedAssetEnabled, | |||
| differentMemosSameAccount, | |||
| canCreateQuote, | |||
There was a problem hiding this comment.
The code looks great but there are a few edge cases that make your change tricky and not safe for deployment:
- There are Anchors that only implement SEP-31 without implementing SEP-38. In those cases, the dependency
canCreateQuotewill fail, as well as this whole sep31 test. You'll need to create a new set of tests that covers anchors implementing both SEP-31+SEP-38. You should create a new file for these tests and decide to add it to the list of tests here:
stellar-anchor-tests/@stellar/anchor-tests/src/helpers/test.ts
Lines 346 to 351 in 11bf82c
- EDIT: you should check for
quotes_supported: truein the SEP-31+SEP-38 tests
- Additionally, if you want to support anchors with
quotes_required: true, you should update thesep31/transactions#canCreateTransactiontest by checking if they are required, and if that's the case, make the same request as the code was doing before and validate if the resulting status code is400.
@JakeUrban: do you have any other input here?
There was a problem hiding this comment.
I've made the following changes:
- Added a set of SEP31+SEP38 tests that execute when
quotes_supported: true - Added a check for
quotes_requiredin SEP31 tests. If it is false,canCreateTransactionand it's dependencies if are executed. Ifquotes_required: true,canCreateTransactionexpects a 400, and its dependencies don't check for anything.
|
Preview is available here: |
|
Preview is available here: |
|
Preview is available here: |
| // If quotes are required, ignore this test, this will be addressed in SEP 31+38 tests | ||
| if (quotesRequired) { | ||
| await makeRequest(postTransactionsCall, 400, result, "application/json"); | ||
| this.context.provides.transactionId = null; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
So this is testing that if quotes are required, that the POST /transactions call fails with a 400? Maybe we should add a failure mode that describes this, that way its clear to the user why the test failed.
There was a problem hiding this comment.
I see what you're saying, but I think we also don't want this test to fail when it's being used for validation if a 400 is the expected behavior.
| const splitAsset = | ||
| this.context.expects.sep38QuoteResponseObj.sell_asset.split(":", 3); |
There was a problem hiding this comment.
Are we verifying the format of this is correct before using it?
There was a problem hiding this comment.
Yes, this has been tested.
|
|
||
| let assetCode = config.assetCode; | ||
| if (assetCode === undefined) { | ||
| throw "asset not configured"; |
There was a problem hiding this comment.
I didn't know you could just throw a string like this in javascript. TIL!
|
Please don't forget to prepare for a new version shipment by:
You can do these changes in this same pull request, so you can ship things faster. After you do that and merge this PR, you just need to create a new release with the. same version number you used in package.json and the same text you used in the changelog. |
|
Preview is available here: |
Specify quote_id in transaction POST request.