Implement allowed wallets feature.

[3m1n3nc3]

This PR allows wallets to be marked as allowed during a pay() transaction, this will prevent the system from attempting to charge from wallets which have not been added to the allowed array of wallets, a possible use case for this is for an escrow system where you may want to restrict the outflow of funds from the escrow wallet before a deal is closed.

See #8

[HPWebdeveloper]

@3m1n3nc3 can you rebase this? I tried, but I saw it has a conflict. I think you can resolve it.

[3m1n3nc3]

@3m1n3nc3 can you rebase this? I tried, but I saw it has a conflict. I think you can resolve it.

@HPWebdeveloper I resolved the errors, you probably missed it.

[HPWebdeveloper]

@3m1n3nc3 I will check it

[3m1n3nc3]

@3m1n3nc3 I will check it

You never merged this pull request, I've never used the main package in my projects because of this missing feature, just the branch I made the pull request from as a vcs, maybe the feature is too opinionated and may never be needed by anyone else but so was the custom reference feature, you may never know.

[HPWebdeveloper]

@3m1n3nc3

This is not an opinionated feature.

Could you please confirm my understanding. And clarify those point I misunderstood or it is not implemented. Thank you.

The explanation in the PR suggests adding a feature to the package where only certain wallets can be used for making payments during a transaction. This means you can specify which wallets are permitted to be charged, and the system will ignore any wallets not included in this list. This can be useful in scenarios like an escrow system, where you want to ensure funds are not released from the escrow wallet until a specific condition (like closing a deal) is met.

Here's a breakdown of the key points:

• Allowed Wallets Feature: This feature will enable marking certain wallets as "allowed" for transactions.

• Payment Restriction: During a pay() transaction, the system will check if the wallet is in the allowed list before proceeding with the payment.

• Use Case - Escrow System: In an escrow system, you may want to restrict payments from the escrow wallet until certain conditions are met. This feature helps enforce that restriction.

To clarify, this feature ensures that during a transaction, only wallets that are explicitly allowed will be considered for making payments. This prevents unauthorized or premature transactions from certain wallets, providing better control over fund flow, especially in sensitive scenarios like escrow arrangements.

[3m1n3nc3]

@3m1n3nc3

This is not an opinionated feature.

Could you please confirm my understanding. And clarify those point I misunderstood or it is not implemented. Thank you.

The explanation in the PR suggests adding a feature to the package where only certain wallets can be used for making payments during a transaction. This means you can specify which wallets are permitted to be charged, and the system will ignore any wallets not included in this list. This can be useful in scenarios like an escrow system, where you want to ensure funds are not released from the escrow wallet until a specific condition (like closing a deal) is met.

Here's a breakdown of the key points:

• Allowed Wallets Feature: This feature will enable marking certain wallets as "allowed" for transactions.
• Payment Restriction: During a pay() transaction, the system will check if the wallet is in the allowed list before proceeding with the payment.
• Use Case - Escrow System: In an escrow system, you may want to restrict payments from the escrow wallet until certain conditions are met. This feature helps enforce that restriction.

To clarify, this feature ensures that during a transaction, only wallets that are explicitly allowed will be considered for making payments. This prevents unauthorized or premature transactions from certain wallets, providing better control over fund flow, especially in sensitive scenarios like escrow arrangements.

All your key points are correct except the second one, which is partially correct, in this case the payment will still be completed if all the allowed wallets contain sufficient balance to cover the cost, only the wallets missing from the allowed wallets will be ignored.

For context, this is how I use the feature in one of my most recent project.

[HPWebdeveloper]

Okay,

• in your screenshot there is a comment including nia, is this a prefix of your wallet? can you rewrite your comment with wallet_1 and ... wallet_2 hence it is more aligned with the readme and I can understand better.

• if this is merged then the api of payand deposit methods, I mean signatures, will new signature be required always for all transaction or it would be optional? the concern is backward compatibility.

• you have added a comment when deposit, it is required? I need to know the use case

• At this moment (the current vesion) when we pay it returns null, it is not good, and I want to return a meaningful message (this is not related to this PR). Now this PR put some restriction. Hence returning a meaningful message is a demand specifically when it is not allowed. I am sure you have considered this in your PR.

• In this PR, when we pay we need to provide the name of the wallet, if the name is not correct, it should understand.

[3m1n3nc3]

Okay,

• in your screenshot there is a comment including nia, is this a prefix of your wallet? can you rewrite your comment with wallet_1 and ... wallet_2 hence it is more aligned with the readme and I can understand better.
• if this is merged then the api of payand deposit methods, I mean signatures, will new signature be required always for all transaction or it would be optional? the concern is backward compatibility.
• you have added a comment when deposit, it is required? I need to know the use case
• At this moment (the current vesion) when we pay it returns null, it is not good, and I want to return a meaningful message (this is not related to this PR). Now this PR put some restriction. Hence returning a meaningful message is a demand specifically when it is not allowed. I am sure you have considered this in your PR.
• In this PR, when we pay we need to provide the name of the wallet, if the name is not correct, it should understand.

The screenshot is not part of this library, it's from a project I'm worked on but using this library as the wallet system (I am using the branch on my repo as a vcs since the pull request is not yet closed)

The pull request has nothing to do with the deposit() method, just the pay() method, in my screenshot, I am transfering funds from the system_wallet to another wallet, nia_wallet, hence the need to add the system_wallet to the allowed wallets during the payout ($app->pay($request->amout, ['system_wallet'], 'Funding for NIA wallet')), this is important because we don't want funds to be removed from the nia_wallet incase the system_wallet doesn't have sufficient funds since we are going to pay the $request->amout to it.

This PR does not have any side effects besides the fact that it will throw the InsufficientBalanceException if the wallets in the allowed wallets array do not contain enough balance, hence it will not affect your "meaningful message" feature.

SUGGESTION: Instead of returning a meaningful message from the pay method, I suggest that it should return all WalletsLog related and created during the transaction, any developer using the library can easily make a custom message but in my case I have to do WalletsLog::latest()->first() in an attempt to get the log created for the transaction (I happen to need this feature most of the time) which is not ideal in a traffic intensive app.

[3m1n3nc3]

Okay,

• in your screenshot there is a comment including nia, is this a prefix of your wallet? can you rewrite your comment with wallet_1 and ... wallet_2 hence it is more aligned with the readme and I can understand better.
• if this is merged then the api of payand deposit methods, I mean signatures, will new signature be required always for all transaction or it would be optional? the concern is backward compatibility.
• you have added a comment when deposit, it is required? I need to know the use case
• At this moment (the current vesion) when we pay it returns null, it is not good, and I want to return a meaningful message (this is not related to this PR). Now this PR put some restriction. Hence returning a meaningful message is a demand specifically when it is not allowed. I am sure you have considered this in your PR.
• In this PR, when we pay we need to provide the name of the wallet, if the name is not correct, it should understand.

About backwards compatibility, apps will be broken if the developer is already implementing the Transaction Info feature, I intended that this PR was going to be part of v2.0.0 so as to account for the breaking change.

[HPWebdeveloper]

Okay,

• in your screenshot there is a comment including nia, is this a prefix of your wallet? can you rewrite your comment with wallet_1 and ... wallet_2 hence it is more aligned with the readme and I can understand better.
• if this is merged then the api of payand deposit methods, I mean signatures, will new signature be required always for all transaction or it would be optional? the concern is backward compatibility.
• you have added a comment when deposit, it is required? I need to know the use case
• At this moment (the current vesion) when we pay it returns null, it is not good, and I want to return a meaningful message (this is not related to this PR). Now this PR put some restriction. Hence returning a meaningful message is a demand specifically when it is not allowed. I am sure you have considered this in your PR.
• In this PR, when we pay we need to provide the name of the wallet, if the name is not correct, it should understand.

About backwards compatibility, apps will be broken if the developer is already implementing the Transaction Info feature, I intended that this PR was going to be part of v2.0.0 so as to account for the breaking change.

that means having new tag 3.0.0, No problem.

A question, I want pay signature will be like this, if I provide a list of wallets then it deducts from the provided list, if not it does like the current functionality. Is that possible?

[3m1n3nc3]

Okay,

• in your screenshot there is a comment including nia, is this a prefix of your wallet? can you rewrite your comment with wallet_1 and ... wallet_2 hence it is more aligned with the readme and I can understand better.
• if this is merged then the api of payand deposit methods, I mean signatures, will new signature be required always for all transaction or it would be optional? the concern is backward compatibility.
• you have added a comment when deposit, it is required? I need to know the use case
• At this moment (the current vesion) when we pay it returns null, it is not good, and I want to return a meaningful message (this is not related to this PR). Now this PR put some restriction. Hence returning a meaningful message is a demand specifically when it is not allowed. I am sure you have considered this in your PR.
• In this PR, when we pay we need to provide the name of the wallet, if the name is not correct, it should understand.

About backwards compatibility, apps will be broken if the developer is already implementing the Transaction Info feature, I intended that this PR was going to be part of v2.0.0 so as to account for the breaking change.

that means having new tag 3.0.0, No problem.

A question, I want pay signature will be like this, if I provide a list of wallets then it deducts from the provided list, if not it does like the current functionality. Is that possible?

Yes, that's exactly how it works. If no wallet list is provided it falls back to the default functionality.

[HPWebdeveloper]

SUGGESTION: Instead of returning a meaningful message from the pay method, I suggest that it should return all WalletsLog related and created during the transaction, any developer using the library can easily make a custom message but in my case I have to do WalletsLog::latest()->first() in an attempt to get the log created for the transaction (I happen to need this feature most of the time) which is not ideal in a traffic intensive app.

Your suggestion seems good.

Can you work on this specific return general response ( I suggest that it should return all WalletsLog related and created during the transaction,) in a separate PR?

[3m1n3nc3]

SUGGESTION: Instead of returning a meaningful message from the pay method, I suggest that it should return all WalletsLog related and created during the transaction, any developer using the library can easily make a custom message but in my case I have to do WalletsLog::latest()->first() in an attempt to get the log created for the transaction (I happen to need this feature most of the time) which is not ideal in a traffic intensive app.

Your suggestion seems good.

Can you work on this specific return general response ( I suggest that it should return all WalletsLog related and created during the transaction,) in a separate PR?

Yeah sure