-
Notifications
You must be signed in to change notification settings - Fork 33
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
SDFSOF-123: End2End tests for SEP-31 and sync with develop #1076
SDFSOF-123: End2End tests for SEP-31 and sync with develop #1076
Conversation
### What Renaming of RPC actions to RPC requests/methods ### Why Request/method is more general term than action
…tellar-anchor-sdk into fireblocks_integration
…to pending_anchor/pending_received when the stellar transction with requested memo and amount completed on the network (#17) ### What Find transaction by `toAccount` + `memo` combination to handle payment ### Why `SEP-31` payments are not handled correctly
…tellar-anchor-sdk into fireblocks_integration
### What Validation for deposit info generator type ### Why If custody integration is enabled, then only `custody` type should be available. If custody integration is disabled, then `custody` type should be not available.
…dk into fireblocks_integration
### Description End2End tests for SEP-31 ### Context There are no End2End tests for SEP-31
…r-anchor-sdk into fireblocks_integration
Reference Server Preview is available here: |
Hi, @Ifropc It looks like the validation of callbacks is invalid in End2End tests and callbacks themselves also work partially incorrectly.
Tests don't fail, because code, that should assert the error, is not executed, since We also added code to validate callbacks in our new End2End test for custody/RPC/SEP-31, but commented it and left TODOs to investigate this problem. In the examples below, there are 4 events, but 8 callbacks(with duplicates and some requests are missing). Example of events:
Example of callbacks:
|
Reference Server Preview is available here: |
…r-anchor-sdk into fireblocks_integration
Reference Server Preview is available here: |
expectedEvents: List<AnchorEvent>, | ||
actualEvents: List<AnchorEvent> | ||
) { | ||
expectedEvents.forEachIndexed { index, expectedEvent -> |
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.
Why do we modify expectedEvent before comparing it to actualEvent?
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.
Because some fields are dynamic(for example dates) and can not be specified in expectedEvent
. The same logic is used in existing End2End tests - Sep24End2EndTests
actualEvent.transaction.amountIn?.let { | ||
expectedEvent.transaction.amountIn.amount = actualEvent.transaction.amountIn.amount | ||
expectedEvent.transaction.amountIn.asset = asset.sep38 | ||
// expectedEvent.transaction.amountIn.asset = |
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.
Some fileds assignment is commented-out, is it expected?
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 removed this commented code
) | ||
} | ||
} catch (e: ClientException) { | ||
log.error(e) | ||
call.respond(ErrorResponse(e.message!!)) | ||
call.respond(MessageResponse(e.message!!)) |
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.
Why ErrorRespond was changed to MessageRespond in places where exceptions are caught?
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.
It was renamed from ErrorResponse
to MessageResponse
just because it has a field called msg
and I wanted to add a new error response with error
field. But now it looks like it is redundant, so I will use only the old ErrorResponse
Reference Server Preview is available here: |
…dk into fireblocks_integration
Reference Server Preview is available here: |
Fix validation of callbacks in E2E tests
Reference Server Preview is available here: |
…tellar-anchor-sdk into fireblocks_integration # Conflicts: # platform/src/test/kotlin/org/stellar/anchor/platform/service/PaymentOperationToEventListenerTest.kt
Description
SEP-31
flowdevelop
branchTesting
./gradlew test
CUSTODY_TYPE=fireblocks CUSTODY_FIREBLOCKS_VAULT_ACCOUNT_ID=<your_value> SECRET_CUSTODY_FIREBLOCKS_API_KEY=<your_value> SECRET_CUSTODY_FIREBLOCKS_SECRET_KEY=<your_value> ./gradlew clean testFireblocksE2E