Atomic Loans Additions to CAL with LoanFund and Loan Provider #184

matthewjablack · 2019-05-19T18:25:51Z

Description

Atomic Loans additions such as LoanFund Provider and Loan Provider

Introduce getMempoolBalance

Submission Checklist 📝

LoanFund Provider

monokh

Some comments.
Since you are now looking to add quite a bit of code to the CAL, I think it's a good time to host the loan related functionality in a self hosted module and then contribute to the existing modules separately. Development should be much more clear in that way.

monokh · 2019-05-20T12:19:51Z

packages/bitcoin-bitcoinjs-lib-swap-provider/package.json

@@ -1,8 +1,8 @@
 {
-  "name": "@liquality/bitcoin-bitcoinjs-lib-swap-provider",


I think there are some unneeded changes here? 😄

monokh · 2019-05-20T12:22:38Z

packages/client/lib/Chain.js

@@ -180,6 +180,14 @@ export default class Chain {
    return balance
  }

+  async getMempoolBalance (addresses) {


These methods shouldn't be in the interface as they are not cross chain. Are you having issues accessing them internally in the providers using getMethod?

@mattBlackDesign are you trying to get to eth_pendingTransactions or parity_localTransactions or parity_pendingTransactions via this method?

With above rpc calls, we can do something similar on eth side.

monokh · 2019-05-20T12:25:04Z

packages/client/lib/Client.js

@@ -148,6 +152,14 @@ export default class Client {
  get collateral () {
    return this._collateral
  }
+
+  get loan () {


what's the difference with the collateral side? It seems we are expanding the namespaces quite quickly for your usecase. Perhaps it could all come under loan and then you can have sub namespaces like loan.fund.xxxxxx``loan.collateral.xxxxxx ?

monokh · 2019-05-20T12:28:37Z

packages/ethereum-erc20-provider/lib/EthereumErc20Provider.js

+    return isNaN(balances) ? 0 : balances
+  }
+
+  async getErc20Name () {


Not really sure why we need this sort of functionality within the CAL. They cannot be replicated on any other chain therefore I think whichever the standard way of doing this in ethereum world would be a better approach than implementing all of this into the CAL.

harshjv · 2019-05-20T13:10:57Z

@mattBlackDesign @monokh instead of hosting them separately, they can host it here and leverage our build system. If the changes are specifically made to their subpackages, we can let them merge it on their schedule and do a combine review when it is related to the common packages.

For now, the diff is very noisy because of unnecessary changes liquality to atomicloans. @mattBlackDesign can you first fix it? It will be easy to review then and we can quickly move forward.

monokh · 2019-05-20T13:51:16Z

Going to be quite blunt here because I think this can prevent a great deal of headaches in the future.

There is a clear separation of responsibility between the loan providers and the rest of the codebase. The current set of contributors shouldn't be concerned with maintaining or engaging in any real way with the loan related functionality. There are several areas where disagreements and conflict can and will occur in the future if this github project becomes a playground for every addition to the CAL:

Code access writes
Loans contributors shouldn't be required to go through approval to get code merged into the repo. Similarly, the other contributors shouldn't be distracted with changes that don't concern them, however small that may be.

Code responsibility
Every independent module of CAL should be free to tag, release, branch on their own schedule. This one repo system not only accumulates a lot of unrelated components, it also slows down the concerning teams with delivering.
What happens when a loan module fails the build? Who is responsible?
What happens when there is a new version release required immediately but core contributors are not there?
What happens when a non core contributor releases a version of the library which breaks certain things. Who is responsible?

Infrastructure responsibility
Infrastructure (building, deploying) has been setup on the repo for the project in a way that suits a given team. The build system and it's components (docs, releases etc.) may change with no notice to a party. At the same time, a party may want to customise these details, at which point, the infrastructure can become messy.

Discussion and content
Things like READMes, PRs, issues and github pages are unique content between loans and the rest of the project. Mixing it will make everything hard to find and organise.

matthewjablack · 2019-05-20T19:21:55Z

@harshjv removed scope @AtomicLoans to reduce the number of diffs. This was in place temporarily to allow for those packages to be deployed to npm for atomic loans ui.

You have both brought up very good points, and I agree that the loan functionality really has nothing to do with the rest of CAL. This could technically be a completely separate module, in a completely separate repo that is used alongside CAL, however we would need to determine how we want to setup the @liquality/client module to accommodate for that.

We'll also need to think about how modules are published. I personally think that in the future all the modules will probably be their own repos, but I don't think we're at that stage yet.

harshjv · 2019-05-21T07:07:54Z

packages/bitcoin-ledger-provider/lib/BitcoinLedgerProvider.js

@@ -176,7 +176,7 @@ export default class BitcoinLedgerProvider extends LedgerProvider {
      if (inputs && outputs) {
        let change = outputs.filter(output => output.id !== 'main')

-        if (change) {
+        if (change && change.length > 0) {


Nice catch man!

Did you get any absurdly-high-fee error without this?

Sorry for late reply @harshjv.

Yes, in fact it wouldn't actually send the transaction because the fee was too high 😂

Adding this to separate PR: #186

harshjv · 2019-05-21T07:11:42Z

packages/client/lib/Chain.js

@@ -180,6 +180,14 @@ export default class Chain {
    return balance
  }

+  async getMempoolBalance (addresses) {


@mattBlackDesign are you trying to get to eth_pendingTransactions or parity_localTransactions or parity_pendingTransactions via this method?

With above rpc calls, we can do something similar on eth side.

harshjv · 2019-05-21T07:14:15Z

packages/client/lib/Chain.js

@@ -226,4 +234,16 @@ export default class Chain {
  async getAddressMempool (addresses) {


Can you explain why getAddressMempool, getCode erc20Approve, getTransactionReceipt are needed on client.chain object?

matthewjablack added 6 commits May 19, 2019 14:24

Atomic Loans Additions to CAL with LoanFund and Loan Provider

663e7ed

Temporary commit for publish

1939f2c

temporary changes

5c947b8

Update package.json

4370878

Update schema requirements

e7f8234

Test package.json info

ab9c3b6

monokh requested changes May 20, 2019

View reviewed changes

matthewjablack added 5 commits May 20, 2019 14:57

Move namespace back to @liquality

cf08c4e

Fix lerna version

38d1c7b

Fix scope to @liquality

9ea62b5

Fix lerna versions

e7e4789

Update package.json author

07b9d6c

harshjv requested changes May 21, 2019

View reviewed changes

matthewjablack closed this Jun 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Atomic Loans Additions to CAL with LoanFund and Loan Provider #184

Atomic Loans Additions to CAL with LoanFund and Loan Provider #184

matthewjablack commented May 19, 2019 •

edited

monokh left a comment

monokh May 20, 2019

monokh May 20, 2019

harshjv May 21, 2019

monokh May 20, 2019

monokh May 20, 2019

harshjv commented May 20, 2019

monokh commented May 20, 2019

matthewjablack commented May 20, 2019

harshjv May 21, 2019

matthewjablack Jun 4, 2019

matthewjablack Jun 4, 2019

harshjv May 21, 2019

harshjv May 21, 2019

		@@ -1,8 +1,8 @@
		{
		"name": "@liquality/bitcoin-bitcoinjs-lib-swap-provider",

		@@ -226,4 +234,16 @@ export default class Chain {
		async getAddressMempool (addresses) {

Atomic Loans Additions to CAL with LoanFund and Loan Provider #184

Atomic Loans Additions to CAL with LoanFund and Loan Provider #184

Conversation

matthewjablack commented May 19, 2019 • edited

Description

Submission Checklist 📝

monokh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshjv commented May 20, 2019

monokh commented May 20, 2019

matthewjablack commented May 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewjablack commented May 19, 2019 •

edited