Form Integrations & KYC Support #27
Conversation
|
@msfeldstein hold off on the review -- the KYC design exposed a flaw in the form integrations design. Once thats adjusted you can review. |
JakeUrban
force-pushed
the
form-integrations
branch
from
ccc6463
to
ec923d5
Compare
|
Ok @msfeldstein This is ready for a review |
JakeUrban
force-pushed
the
form-integrations
branch
6 times, most recently
from
c0976a0
to
c1ad6ea
Compare
|
We've decided to redesign parts of this feature, which will cover KYC use cases. The review can wait until this redesign is pushed. |
JakeUrban
force-pushed
the
form-integrations
branch
3 times, most recently
from
92278bc
to
ea825cb
Compare
JakeUrban
force-pushed
the
form-integrations
branch
from
84384b9
to
6ab77d4
Compare
JakeUrban
force-pushed
the
form-integrations
branch
from
6ab77d4
to
4f7e872
Compare
Codecov Report
@@ Coverage Diff @@
## master #27 +/- ##
========================================
+ Coverage 78.79% 79.2% +0.4%
========================================
Files 20 20
Lines 849 880 +31
========================================
+ Hits 669 697 +28
- Misses 180 183 +3
Continue to review full report at Codecov.
|
docs/integrations/index.rst
Outdated
| Polaris does most of the work implementing SEP-24_. However, some pieces of | ||
| SEP-24 can only be implemented by the anchor. Specifically, anchors need to | ||
| implement their own banking rails and KYC requirements. This is where | ||
| :class:`DepositIntegration` and :class:`WithdrawalIntegration` come in. |
There was a problem hiding this comment.
"This is where the Integrations classes come in" since KYC is added now and Deposit/WithdrawIntegration doesn't cover all the integrations
docs/integrations/index.rst
Outdated
| the box with forms for deposit and withdrawal flows. | ||
|
|
||
| However, the data collected may not be sufficient for your needs. Maybe you | ||
| need to do collect additional fields and some validation or processing with |
There was a problem hiding this comment.
s/Maybe you need to do collect/Maybe you need to collect
| form = rdi.form_for_transaction(transaction)(request.POST) | ||
| is_transaction_form = issubclass(form.__class__, TransactionForm) | ||
| if is_transaction_form: | ||
| form.asset = asset |
There was a problem hiding this comment.
Why do we need the asset to live in the form? It doesn't seem like we'd want to show this as a form element to the user, its not something they can change
There was a problem hiding this comment.
form.asset is used in the validation function for the amount field, which is called from form.is_valid(). We don't control the parameters for the validation function so we have to give the form access to the non-field object somehow.
asset is not shown in the form, only attributes of type Field are displayed.
| form_class = rdi.form_for_transaction(transaction) | ||
|
|
||
| if form_class: | ||
| return Response( |
There was a problem hiding this comment.
I get the feeling the GET and POST methods should be different endpoints, both for code clarity, but also in case the page needs to reload.
For example POST /webapp/submit_form will process the request then redirect to GET /webapp to show the next available form. Otherwise, if the page needs to reload for any reason the browser/webview will want to rePOST the previous form.
I think this is not a big deal for now but we may want to change this in the future
There was a problem hiding this comment.
Thats fair, I'll make a separate PR for it. #34
| @@ -19,8 +26,8 @@ def poll_pending_deposits(cls, pending_deposits: QuerySet | |||
| **OVERRIDE REQUIRED** | |||
|
|
|||
| This function should poll the financial entity for the state of all | |||
| `pending_deposits` transactions and return the ones ready to be | |||
| executed on the Stellar network. | |||
| `pending_deposits` and return the ones ready to be executed on the | |||
There was a problem hiding this comment.
Clarify that it returns the ones where the external deposit has been completed. "ready to be executed on the stellar network" implies that the external deposit has been completed but it couldn't help to be explicit
| @@ -45,6 +53,7 @@ def poll_pending_deposits(cls, pending_deposits: QuerySet | |||
| memory usage. | |||
|
|
|||
| :param pending_deposits: a django Queryset for pending Transactions | |||
| :return: a list of Transaction database objects to execute | |||
There was a problem hiding this comment.
a list of Transaction database objects to execute feels confusing, it sounds like its going to execute a database transaction, it should be explicit that its Transaction entries for transactions which have successful user deposits and are ready to be executed on the stellar network
| @@ -16,13 +16,18 @@ | |||
|
|
|||
| {% for field in form %} | |||
| <div class="field"> | |||
| {% if field.name == 'amount' %} | |||
There was a problem hiding this comment.
Is fa-dollar-sign a bulma attribute? If so, can we pass it in in the django.Form instead of special casing amount here?
There was a problem hiding this comment.
I don't think so because the <span> tag and its contents are a separate component from the form field. The fa-dollar-sign attribute probably needs to live in an <i> tag too.
Docs
Design
TransactionFormbase classDepositIntegrationandWithdrawalIntegrationform_for_transaction()after_form_validation()