Skip to content
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

Two-Part Payment Support #341

Merged
merged 51 commits into from
Oct 26, 2020

Conversation

Zagan202
Copy link
Contributor

@Zagan202 Zagan202 commented Oct 16, 2020

Description

Adds two part payments (aka claimableBalances) to the deposit flow.
In the past when accounts attempt a deposit

Old Flow

  1. The Wallet will initiate a deposit to an account that does not exist yet,
    or the user's account won't have a trustline to the asset's issuer account.
  2. Polaris will be run periodically via the poll_pending_deposits
  3. Polaris calls DepositIntegration.create_channel_account() for the transaction record (which creates a temporary, per-deposit-transaction-object Stellar account)
  4. Polaris will then mark those created accounts as pending_trust
  5. The Wallet will then communicate to the user that the transaction could not be completed until a trust line is established.
    for their own account or the one that was created for them
  6. Polaris will call periodically check_trustlines (a command line tool that periodically checks if the
    transactions with this status now have a trustline to the relevant asset).
  7. When the user establishes a trustline Polaris will submit the transaction to the stellar network and call the
    after_deposit integration function once its completed.

Desired Flow

  1. The Wallet will initiate a deposit to an account that does not exist yet,
    or the user's account won't have a trustline to the asset's issuer account.
  2. Polaris will be run periodically via the poll_pending_deposits
  3. Polaris calls DepositIntegration.create_channel_account() for the transaction record (which creates a temporary, per-deposit-transaction-object Stellar account)
  4. Polaris will then mark those created accounts as pending_trust
  5. Polaris will be run periodically via the poll_pending_deposits
  6. Polaris will create a stellar deposit (create_stellar_deposit) create_transaction_envelope where it will build the transaction as a claimableBalance
  7. Submit the transaction and mark the status as completed.

Addresses issue: #316
resolves stellar/stellar-protocol#747

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

  • tests/processes/test_create_stellar_deposit.py

- create_transaction_envelope now takes the claimable flag which
indicates whether or not the transaction should be built as a claimableBalance

Signed-off-by: Lawrence <[email protected]>
- based on the SDK's exammple
(py-stellar-base/examples/claimable_balances.py)
- Currently the claimableBalance created has is an `UNCONDITIONAL`
predicate
  - This means that there is no stated condition for the reciever to
  claim the balance https://developers.stellar.org/docs/glossary/claimable-balance/#relevant-operations

Signed-off-by: Lawrence <[email protected]>
- Now that we dont fail if no trustline is availible we reflect successful
 responses like test_deposit_stellar_success

Signed-off-by: Lawrence <[email protected]>
- We have no need for this code due to the ability to create
claimableBalances for those recipiants (to claim at a later date)
without a trustline established.

Signed-off-by: Lawrence <[email protected]>
Signed-off-by: Lawrence <[email protected]>
- create_transaction_envelope now takes the claimable flag which
indicates whether or not the transaction should be built as a claimableBalance

Signed-off-by: Lawrence <[email protected]>
- based on the SDK's exammple
(py-stellar-base/examples/claimable_balances.py)
- Currently the claimableBalance created has is an `UNCONDITIONAL`
predicate
  - This means that there is no stated condition for the reciever to
  claim the balance https://developers.stellar.org/docs/glossary/claimable-balance/#relevant-operations

Signed-off-by: Lawrence <[email protected]>
- Now that we dont fail if no trustline is availible we reflect successful
 responses like test_deposit_stellar_success

Signed-off-by: Lawrence <[email protected]>
- We have no need for this code due to the ability to create
claimableBalances for those recipiants (to claim at a later date)
without a trustline established.

Signed-off-by: Lawrence <[email protected]>
Signed-off-by: Lawrence <[email protected]>
Signed-off-by: Lawrence <[email protected]>
Copy link
Contributor

@JakeUrban JakeUrban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start, at a high level everything looks good here.

The key feature I'd like to see is a Transaction.claimable_balance_id column that gets populated on successful TX submission.

Other than that, there are some small adjustments to make, and I also think we could add to the documentation to explain when Claimable Balances are used in Polaris.

polaris/polaris/utils.py Outdated Show resolved Hide resolved
polaris/polaris/utils.py Outdated Show resolved Hide resolved
polaris/polaris/utils.py Outdated Show resolved Hide resolved
polaris/polaris/utils.py Outdated Show resolved Hide resolved
- Because the pending_trust transactions are the objects we want to
create a claimableBalance from we can simply check the transaction status

Signed-off-by: Lawrence <[email protected]>
whoops

Signed-off-by: Lawrence <[email protected]>
- set transaction.claimable_balance_id with get_balanceid
only if the transaction is pending trust at the time it was built

Signed-off-by: Lawrence <[email protected]>
- I generated a result_xdr for get_balanceid to decode when this test runs

Signed-off-by: Lawrence <[email protected]>
- This is the default value

Signed-off-by: Lawrence <[email protected]>
- there is more than one operation we want to ensure we dont fail if
createClaimableBalanceResult is the 1st..nth tr in the xdr
- This will return too early in the case where there are multiple
createClaimableBalanceResult operations on a singe transaction
  - TODO: change the model attr to be an array of strings?

Signed-off-by: Lawrence <[email protected]>
- With the ability to create claimable balances we dont need to specify if the
destination exists. We simply always call
get_or_create_transaction_destination_account with this new flow.

Signed-off-by: Lawrence <[email protected]>
- test_deposit_stellar_no_account no longer tests a blocked payment flow

Signed-off-by: Lawrence <[email protected]>
Copy link
Contributor

@JakeUrban JakeUrban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks good pending my comments.

Also: Did you add black to your git commit hook?

polaris/polaris/models.py Outdated Show resolved Hide resolved
polaris/polaris/tests/sep24/test_deposit.py Outdated Show resolved Hide resolved
polaris/polaris/utils.py Outdated Show resolved Hide resolved
polaris/polaris/utils.py Outdated Show resolved Hide resolved
- result_xdr value isnt representative of the other deposits made however this will not block or confuse other tests

Signed-off-by: Lawrence Lawson <[email protected]>
Signed-off-by: Lawrence Lawson <[email protected]>
Signed-off-by: Lawrence Lawson <[email protected]>
@Zagan202
Copy link
Contributor Author

Nice, looks good pending my comments.

Also: Did you add black to your git commit hook?

I did now haha

@JakeUrban
Copy link
Contributor

@Zagan202 thanks, can you format the code in a commit and push? I think you just need to change whitespace and black will format the code prior to commit.

@Zagan202
Copy link
Contributor Author

@Zagan202 thanks, can you format the code in a commit and push? I think you just need to change whitespace and black will format the code prior to commit.

Thats strange everything seemed to check out on my end
I'm getting black....................................................................Passed
Could I see what your .pre-commit-config.yaml is?

@Zagan202
Copy link
Contributor Author

Also ran pre-commit run --all-files for good measure.
All the failing files (then formatted) are ones I didn't make changes to.
Would you want me to format and commit those?
I thought its ideal to keep this PR focused on changes that directly impact the issue,

pre-commit run --all-files                            
black....................................................................Failed
- hook id: black
- files were modified by this hook

reformatted /Users/lawrence/stellar-dev/django-polaris/polaris/polaris/integrations/javascript.py
reformatted /Users/lawrence/stellar-dev/django-polaris/example/server/migrations/0003_auto_20200222_0003.py
reformatted /Users/lawrence/stellar-dev/django-polaris/example/server/migrations/0005_auto_20200702_1908.py
reformatted /Users/lawrence/stellar-dev/django-polaris/example/server/migrations/0006_auto_20200615_2046.py
reformatted /Users/lawrence/stellar-dev/django-polaris/polaris/polaris/tests/processes/test_watch_transactions.py
reformatted /Users/lawrence/stellar-dev/django-polaris/polaris/settings.py
reformatted /Users/lawrence/stellar-dev/django-polaris/example/server/settings.py
reformatted /Users/lawrence/stellar-dev/django-polaris/polaris/polaris/management/commands/watch_transactions.py
reformatted /Users/lawrence/stellar-dev/django-polaris/polaris/polaris/shared/endpoints.py
reformatted /Users/lawrence/stellar-dev/django-polaris/polaris/polaris/sep31/transactions.py
reformatted /Users/lawrence/stellar-dev/django-polaris/polaris/polaris/tests/shared_endpoints/test_transactions.py
reformatted /Users/lawrence/stellar-dev/django-polaris/polaris/polaris/tests/sep6/test_deposit.py
reformatted /Users/lawrence/stellar-dev/django-polaris/polaris/polaris/migrations/0001_initial.py
reformatted /Users/lawrence/stellar-dev/django-polaris/polaris/polaris/tests/sep12/test_customer.py
reformatted /Users/lawrence/stellar-dev/django-polaris/polaris/polaris/tests/sep31/test_send.py
All done! ✨ 🍰 ✨
15 files reformatted, 111 files left unchanged.

@JakeUrban
Copy link
Contributor

JakeUrban commented Oct 22, 2020

All I ever do is git commit -m ""; git push

(django-polaris) ~/Documents/workspace/stellar/django-polaris (master) $ black --version
black, version 19.10b0
(django-polaris) ~/Documents/workspace/stellar/django-polaris (master) $ cat .git/hooks/pre-commit
echo "Formatting code..."
black polaris
black example
git add .

Signed-off-by: Lawrence Lawson <[email protected]>
Copy link
Contributor

@JakeUrban JakeUrban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@JakeUrban
Copy link
Contributor

We can merge this and simply hold off on including it in a release until anchors are expected to use claimable balances.

@Zagan202 Zagan202 merged commit 4b336fd into stellar:claimable-balance-support Oct 26, 2020
@Zagan202 Zagan202 deleted the 316-2pt-payments branch October 26, 2020 17:45
@Zagan202 Zagan202 mentioned this pull request Nov 30, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SEP-24: Claimable Balance Op Support Add support for two-part payments
3 participants