-
Notifications
You must be signed in to change notification settings - Fork 34
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
[Feature] Merge sep-6
into develop
#1151
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This implements the SEP-6 `GET /deposit` endpoint _mostly_ according to the sequence diagram from the PR. Where the implementation deviates is how `CUSTOMER_UPDATED` anchor events are structured. This implementation sends out 1 event whenever a SEP-12 customer is updated instead of fanning them out a `TRANSACTION_STATUS_CHANGED` event for each ongoing transaction submitted by the customer. This was done to keep the scope of this change small, but we can consider whether to move this into the platform later.
### Description This removes the `ActiveTransactionStore` from the Kotlin reference server. Previously, this was used to track which transactions were pending KYC info updates. Now, we request the transactions from the platform. ### Context This is part of a series of commits to clean up the reference server implementation. ### Testing `./gradlew test` ### Known limitations N/A
### Description This PR implements the GET `/withdraw` endpoint and the reference server implementation so that the E2E tests pass. It also includes minor refactoring to the payment observer. ### Context SEP-6 implementation. ### Testing `./gradlew test` ### Known limitations N/A
### Description This adds a diagram illustrating the withdraw flow in an Anchor Platform deployment. ### Context Knowledge sharing. ### Testing N/A ### Known limitations N/A
### Description This implements the SEP-6 `withdraw-exchange` endpoint. The request object is prefixed with `Get`, all request objects will be renamed in a separate PR. ### Context SEP-6 implementation. ### Testing `./gradlew test` ### Known limitations This PR only includes unit tests. Integration tests and end-to-end tests will follow after this PR is merged.
### Description This fixes the response returned by GET `/info`. ### Context This was discovered after enabling the SEP-6 test suite in `stellar-anchor-tests`. ### Testing - `./gradlew test` - `stellar-anchor-tests` ### Known limitations N/A
### Description This change: - Adds additional validation to SEP-6 deposit - Adds `minAmount` and `maxAmount` validation to all operations - Makes `type` and `amount` optional in `deposit` and `withdraw` - Refactors the validation into its own class ### Context During the testing of `stellar-anchor-tests`, it was discovered that `type` and `amount` are optional parameters for the `deposit` and `withdraw` endpoints. `Sep6Service` had a lot of code duplication so I decided to refactor the validation into its own class. ### Testing - `./gradlew test` - `stellar-anchor-tests` ### Known limitations N/A
### Description This implements the GET `deposit-exchange` endpoint. There's a lot of duplicated code that could be shared with `deposit` and `withdraw`/`withdraw-exchange`. Once the integration and end-to-end tests have been implemented, I will focus on cleaning up the code. ### Context SEP-6 implementation. ### Testing `./gradlew test` ### Known limitations Integration and end-to-end tests are missing, they will be added in another PR.
…eeds (#1136) ### Description This adds a check to verify that a customer is SEP-12 accepted as part of the deposit/withdraw request validation. This also removes the `VERIFICATION_REQUIRED` status as it is not a valid SEP-12 customer info status. ### Context This was caught by a `stellar-anchor-tests` test case. ### Testing - `./gradlew test` - `stellar-anchor-tests` ### Known limitations N/A
### Description This enables client status callback for SEP-6. ### Context Status callback should be supported by the SEP-6 implement. ### Testing `./gradlew test` ### Known limitations The SEP-24 end-to-end was previously not asserting on the wallet status callbacks. This was caused by the signature verification failing in the wallet reference server, causing the GET `/callback` to always return null. The test previously only asserted when the result was _not_ null. These tests have been disabled in this PR due to flakiness but will be re-enabled again when `develop` is merged in.
### Description Since we are validating min/max amounts, if set, these should be show in the GET /info response. ### Context These are currently missing from the info response. ### Testing - `./gradlew test` ### Known limitations N/A
### Description Removes the `to` account set by the withdraw endpoint. ### Context `to` is an optional field in the transactions object, and is meant to store the user's off-chain account. This is not known at the time of transaction creation. ### Testing - `./gradlew test` ### Known limitations N/A
### Description This enables SEP-6 validation tests in the CI workflow. SEP-24 tests have been disabled as they don't pass in the latest version `0.6.x`. They will be re-enabled again once they have been fixed in ANCHOR-487. ### Context Testing SEP-6. ### Testing - `./gradlew test` ### Known limitations N/A
### Description SEP-6 will now check the KYC information of the SEP-10 account instead of the requested account. ### Context The requested account is not the same as the SEP-10 account for custodial wallets. ### Testing - `./gradlew test` ### Known limitations N/A
### Description This sets the `customers`(See `TransactionSEP31` [docs](https://developers.stellar.org/api/anchor-platform/resources/get-transaction)) field in the SEP-6 platform transaction. ### Context When the business server receives a transaction event, it needs to verify if the customer has enough KYC information to proceed with the transaction. Currently, the platform transaction only has `source_account` and `destination_account` fields which is not enough to identify users using a custodial wallet. ### Testing - `./gradlew test` ### Known limitations This persists SEP-12 customer IDs in the SEP-6 transaction table. SEP-12 customers can be deleted at any time, therefore an anchor may lose the mapping from transactions to these customers. This should be fine though since transactions would likely not move back into `pending_customer_info_update` after accepting user funds and can be completed.
…info_updates (#1161) ### Description This updates the SEP-6 E2E test to use the `required_customer_info_updates` field when providing KYC updates. ### Context Currently, this field is not set by the reference server implementation. This is needed so that the demo wallet could prompt the user to submit the missing KYC information. ### Testing - `./gradlew test` ### Known limitations N/A
### Description This sets the `client_id` in the `GET /fee` integration request. It also fixes a bug where the `senderId` and `receiverId`s were incorrectly set in the same request. ### Context Client attribution should work for SEP-6. ### Testing - `./gradlew test` ### Known limitations N/A
### Description This commit adds the `type` field to the SEP-6 platform transaction. ### Context The `type` determines which fields need to be collected for KYC. ### Testing - `./gradlew test` ### Known limitations N/A
### Description This fixes two test concurrency bugs due to static mocking. 1. `Sep10ServiceTest`: the verify sees two calls to `Sep10Challenge#newChallenge`. My guess is that the `LockAndMockStatic` annotation does not know about `ParameterizedTest`s. 2. `Sep24ServiceTest`: the mock of `KeyPair` in `Sep10ServiceTest` to throw an exception causes the `Sep24Service` to throw it as well. These two issues were only observed in the `sep-6` branch despite it using static mocks or touching existing tests. ### Context Fixes test stability in `sep-6` branch. ### Testing - Ran `./gradlew test` a few times ### Documentation N/A ### Known limitations This is only a temporary fix as it does not prevent us from writing similar tests in the future.
### Description This removes KYC verification from the SEP-6 transaction initiation. Now that KYC is removed, the customer may not be known to the Anchor at the time of transaction initiation so the `customer` field cannot be set. To allow Anchors to associate SEP transactions with a customer, the SEP-10 account memo is now added as a field to the `StellarId` object. This means for the exchange endpoints, the platform can no longer make a request to the Fee integration as it requires setting SEP-12 sender and receiver fields. Business servers are expected to update the amounts asynchronously like they already do for regular fees. ### Context KYC verification is done "interactively" in SEP-6. ### Testing - `./gradlew test` ### Documentation Stellar docs need to be updated so that the `StellarId` object includes an optional `memo` field ### Known limitations N/A
### Description This updates the RPC actions for SEP-6 and updates the reference server implementation to use them. This PR is quite large as it updates the unit tests for most of the RPC actions but, implementation-wise, not much has changed. It might helpful to start by reviewing the `Sep6End2EndTest` to get a sense of what changed with this PR. ### Context SEP-6 should support RPC API. ### Testing - `./gradlew test` ### Known limitations - Platform API integration tests do not test the new flow but are covered by the SEP-6 e2e tests. - Refunds and custody service integration have not been implemented. Both of these will be addressed by ANCHOR-508
### Description Cleanly shut down the reference server's Kafka consumer. ### Context I am making this small change to test why the CI passes in PR, but not after merging into `sep-6` branch. ### Testing - `./gradlew test` ### Documentation N/A ### Known limitations N/A
### Description This implements the custody service RPC actions. This includes payments as well as refunds. ### Context SEP-6 will support custodians. ### Testing - `./gradlew test` ### Documentation - [stellar-docs](stellar/stellar-docs#253) ### Known limitations N/A
lijamie98
approved these changes
Nov 3, 2023
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.
LGTM
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This merges the
sep-6
development branch intodevelop
.Context
Anchor Platform is releasing SEP-6 support in 2.4.0.
Documentation
API docs have been updated in stellar/stellar-docs#253. This will merged be merged alongside this PR.
Testing
./gradlew test
Known limitations
N/A