[ANCHOR-388] SEP-6: Implement RPC actions #1166
Conversation
philipliu
force-pushed
the
feature/anchor-388-rpc-api
branch
8 times, most recently
from
001eb85 to
a18779e
Compare
philipliu
force-pushed
the
feature/anchor-388-rpc-api
branch
from
a18779e to
f3005cd
Compare
|
Can we add class comments of the RPC handler classes? Or some reference link to the RPC definition document? |
|
At this moment, I am not familiar with the RPC enough to make too much meaningful reviews. But the PR looks fine with me. |
Sure, I can link to the docs, but I'll do this outside of this PR. |
| @@ -90,6 +90,14 @@ public StartDepositResponse deposit(Sep10Jwt token, StartDepositRequest request) | |||
| .type(request.getType()) | |||
| .assetCode(request.getAssetCode()) | |||
| .assetIssuer(asset.getIssuer()) | |||
| // NB: these are purposely set to incorrect values. | |||
There was a problem hiding this comment.
Does it mean there will be a state of the transaction in DB when it has incorrect amounts?
There was a problem hiding this comment.
Yeah, the amounts are technically incorrect anyway since the Anchor updates fees asynchronously. Deposit instructions and withdraw anchor accounts (to be done in the next PR) will not be provided until the business server makes a request for funds RPC so there is no way for the user to accidentally submit the wrong amount.
There was a problem hiding this comment.
But if the user requests the transaction, won't we return incorrect data? Shouldn't amount just be null instead?
There was a problem hiding this comment.
Yes. the NotifyAmountsUpdatedHandler validation requires the assets to be set so it can't be null (code). I guess we can remove the validation?
There was a problem hiding this comment.
I should probably think about this more, so I'll create a ticket to address this and merge it as is.
There was a problem hiding this comment.
I'm pretty sure RPC validation should just validate only the request having amount to not be null, if we expect transaction to start with null amounts
There was a problem hiding this comment.
Yeah, that makes sense. Created ANCHOR-525 to fix it.
| @@ -165,18 +200,26 @@ NotifyOffchainFundsReceivedHandler notifyOffchainFundsReceivedHandler( | |||
|
|
|||
| @Bean | |||
| NotifyOffchainFundsSentHandler notifyOffchainFundsSentHandler( | |||
| Sep6TransactionStore txn6Store, | |||
There was a problem hiding this comment.
We def should group this services together to simplify all factory methods for RPC handlers
There was a problem hiding this comment.
Yeah, this was quite annoying...
platform/src/main/java/org/stellar/anchor/platform/rpc/NotifyOffchainFundsReceivedHandler.java
Show resolved
Hide resolved
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
Sep6End2EndTestto get a sense of what changed with this PR.Context
SEP-6 should support RPC API.
Testing
./gradlew testKnown limitations
Both of these will be addressed by ANCHOR-508