Payments pallet

[olanod]

Continuing the discussion in #665. We, the Virto Network team recently finished our first Web3 grant milestone for the development of the payments_pallet, a flexible and economically secure escrow-like system that allows people pay for off-chain products or services with any given MultiReservableCurrency, we believe our pallet can have numerous use cases outside our parachain and being part of the ORML code base makes it more reachable to the community.

Main Features

• Basic escrow-like functionality: when users make a payment funds are transferred to destination account but remain on hold until the user triggers a release(usually when an off-chain event like a bank transfer has taken place)
• Dispute resolution: Each payment is assigned a configurable judge account that can resolve disputes when the parties disagree.
• Fee handler: Users of the pallet can implement their own logic to charge extra fees per payment.
• Payment remarks: Payments can optionally include a "remark" that can give external components some context about what's being payed(e.g. the fee handler can use that info to charge fees based on the remark data)
• Payment request: Recipients can initiate payments that just need to be accepted by the payer. *Not part of Web3 grant
  Recurrent payments: Ability to create payments that repeat periodically TBD. *Not part of Web3 grant

[codecov-commenter]

Codecov Report

Merging #691 (d6502bc) into master (23f7092) will increase coverage by 1.37%.
The diff coverage is 89.81%.

@@            Coverage Diff             @@
##           master     #691      +/-   ##
==========================================
+ Coverage   76.15%   77.52%   +1.37%     
==========================================
  Files          84       88       +4     
  Lines        8065     9003     +938     
==========================================
+ Hits         6142     6980     +838     
- Misses       1923     2023     +100     

Impacted Files	Coverage Δ
payments/src/weights.rs	9.52% <9.52%> (ø)
payments/src/lib.rs	83.79% <83.79%> (ø)
payments/src/mock.rs	95.83% <95.83%> (ø)
payments/src/tests.rs	99.68% <99.68%> (ø)
tokens/src/lib.rs	83.64% <0.00%> (-0.53%)	⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[xlc]

There should be some minimal payment amount to prevent spam

[olanod]

There should be some minimal payment amount to prevent spam

@xlc wouldn't the usual system fee calculated from the weight be enough to protect from spam? It's hard to determine what a reasonable minimum payment amount should be since different tokens can differ greatly in value, but I assume we want to at least ensure the receiving account gets the minimum existential deposit to not reaped? Should MultiCurrency::minimum_balance do the trick? Is the current tokens implementation using that minimum balance to keep the account alive?

[xlc]

The fee is not enough. It only covers the computation, not storage.

The principle is that every storage write must come with some form of spam protection. It can be a fixed amount deposit (like pallet proxy does), or some valuable assets is locked (ED).

ED is usually low and can only cover so much storage usage. So the protection will be weak if you expand ED to cover too much storages and this is just an additional expansion.

Ultimately it should be the runtime to decide the ED value, how much storages it can cover, and if this particular feature is included or not. You shouldn't avoid making decision for the runtime within pallet, which will make it less reusable.

The the simple solution is to expose a hook to check if payment amount so you can defer the decision to the runtime. Then you can simply check against with minimum_balance if you want to. Others could figure out the right number for them.

[olanod]

Understood, reserving an extra deposit for the storage cost of the remark which is our main variable is in our todo list, I was thinking of making it part of the "incentive amount" to not add yet another fee and configuration parameter that hurts usability, incentive amount is an extra percentage reserved from the payment returned after release(when payment detail is removed from storage), it can remain being a percentage but we can require it to not fall bellow a minimum that depends on the size of the remark.

[xlc]

Why do you even need to store the remark onchain? It can be queried easily with an indexer.

Also currently this pallet won't support multiple payments between a sender and receiver, which is maybe fine, but less than ideal.

[olanod]

Fair point about the remark we could change that, I wasn't seeing it like the system remark but a more important part of the payment detail that other pallets could access for whatever reason. In Virto we plan to use it within the FeeHandler, we have the concept of local communities that will collect a tax and marketplaces that are a kind of token curated registry that also get a percentage of the payment, p2p payments outside a marketplace will pay a higher fee that goes to the local community and payments via a marketplace pay a smaller fee(a tiny part still going to the local community) where we use the remark to know the marketplace and potentially the kind of article in case some items in the future will be taxed differently. Since the remark is still in scope while doing the fees calculation(apply_fees will return a list of account->fee) I guess we don't actually need to store it.

The single payment per sender-receiver is actually on purpose, it makes the pallet simpler and it proved better for users actually since its easier to know that you only have one transaction at the time with a given client, at least for p2p interactions that we handle in a chat is simpler and more than acceptable, imagine the treasury paying a team for some development, you usually want them to finish one job before staring another. In the future it might become a bigger limitation but I guess we'll let users be the judges of that :)

[stanly-johnson]

Not sure why the CI fails, make dev-format-check passes without issue on local machine

[xlc]

Not sure why the CI fails, make dev-format-check passes without issue on local machine

That will be due to rust version. Please make sure you are using a later version

open-runtime-module-library/.github/workflows/test.yml

Line 36 in 9e041dc

toolchain: nightly-2021-11-07

[olanod]

Hi @xlc sorry to bother but it seems like you are the only one who can currently help us with this review.
In a related note, a topic for a Substrate seminar popped up about diving into ORML and how to contribute to it, it would be interesting to use this chance to think of ways ORML could be made more community driven and have better contribution guidelines in place that can help bring good quality content to the repository. Would love to hear some thoughts on the subject.

[xlc]

should limit number of tasks to take

[stanly-johnson]

16be330

[xlc]

this commit is lost? also better to use .take(n) than truncate

[xlc]

Can you upgrade to polkadot-v0.9.19 and address my comments and then it should be good to merge