[ANCHOR-464] SEP-6: Enable client status callback #1126
Conversation
philipliu
force-pushed
the
feature/anchor-464-sep-6-status-callback
branch
3 times, most recently
from
7907ffc to
d62052f
Compare
philipliu
force-pushed
the
feature/anchor-464-sep-6-status-callback
branch
from
d62052f to
8d8e7a9
Compare
| * @param txn the SEP-6 database transaction object | ||
| * @return the SEP-6 API transaction object | ||
| */ | ||
| public static org.stellar.anchor.api.sep.sep6.Sep6Transaction fromTxn(Sep6Transaction txn) { |
There was a problem hiding this comment.
| public static org.stellar.anchor.api.sep.sep6.Sep6Transaction fromTxn(Sep6Transaction txn) { | |
| public static Sep6Transaction fromTxn(Sep6Transaction txn) { |
There was a problem hiding this comment.
Unfortunately, we have two classes with the same name.
There was a problem hiding this comment.
Why do we need org.stellar.anchor.api.sep.sep6.Sep6Transaction?
That pattern does not seem to exist for Sep24 and Sep31.
There was a problem hiding this comment.
Ah, it's because SEP-24/31 call the platform transaction class something else. https://github.com/stellar/java-stellar-anchor-sdk/blob/625e469bb7c55ffa77d7cf0906b1b926da967fa0/core/src/main/java/org/stellar/anchor/sep24/Sep24Helper.java#L81.
I will rename the class too.
| } | ||
| val actualCallbacks = | ||
| walletServerClient.pollCallbacks(response.id, 4).map { | ||
| gson.fromJson(it, Sep24GetTransactionResponse::class.java) |
There was a problem hiding this comment.
Can you add some comments of why removing assertEquals(4, it.size) would fix the problem?
There was a problem hiding this comment.
It doesn't fix the problem. The assertion was required before because waitForWalletServerCallbacks returned a null if the response size is not 4. I changed the API of waitForWalletServerCallbacks / pollCallbacks so that it will always return a list, so the assertion can be removed.
| @@ -11,7 +11,6 @@ import kotlin.time.Duration.Companion.seconds | |||
| import kotlinx.coroutines.delay | |||
There was a problem hiding this comment.
💡 Can we separate the fix to a different PR?
There was a problem hiding this comment.
I needed to add tests for SEP-6 client status callbacks which ended up requiring touching the SEP-24 tests.
There was a problem hiding this comment.
These changes will be thrown away once I merge in develop, anyway. I've only modified these tests so the CI passes.
| var retries = 5 | ||
| var callbacks: List<String> = listOf() | ||
| while (retries > 0) { | ||
| // TODO: remove when callbacks are de-duped |
There was a problem hiding this comment.
Where and when the callbacks should be de-duped?
There was a problem hiding this comment.
I'm not sure.... Apparently this was fixed in the develop branch. When I pull that in, I'll remove this TODO and see if everything works.
Description
This enables client status callback for SEP-6.
Context
Status callback should be supported by the SEP-6 implement.
Testing
./gradlew testKnown 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
/callbackto 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 whendevelopis merged in.