pendulum-chain / pendulum

GNU General Public License v3.0
43 stars 14 forks source link

Create automation pallet for EuroBrasil project #357

Open ebma opened 1 year ago

ebma commented 1 year ago

Context

The context can be found in this Notion page.

Pallet configuration

Name

Config Items

Storage Items

Extrinsics

Functions (not exposed as extrinsic but available to other pallets)

Events

Coupling to other pallets

Processing the offramp

Prerequisites

Upon receiving an XCM transfer of BRZ from Moonbeam targeting our automation pallet, the XCM config deposits the respective amount of BRZ into this pallet’s on-chain account (returned by getPalletAccount()) and calls the processEurcOfframp() function, supplying the processed amount of BRZ and the reference number (which is encoded in the MultiLocation).

Implementation of the processEurcOfframp(amount, reference) function

The processEurcOfframp() function checks if there is an entry for the supplied reference parameter in the ReferenceToEurAmount map. If so, it uses the call() function of the Contracts pallet to call the swapExactTokensForTokens() function on Nabla’s router smart contract to swap the amount of BRZ tokens to EURC tokens. As a result, it will receive the corresponding amount of EURC on its on-chain account. The swapExactTokensForTokens() function needs to be called using the addresses of the ERC-20 contracts. The signature of the function can be found here. The amount of EURC tokens received as a result of calling the swapExactTokensForTokens() function is matched against the amount contained in the entry of the ReferenceToEurAmount map. If the resulting amount of the swap is higher than what was expected in the entry, the surplus of EURC tokens is sent to the buffer pool account. If the resulting amount of the swap is less than what was expected in the entry, the pallet tries to transfer the missing amount of tokens from the buffer pool to the pallet account. If the buffer pool does not have enough tokens to cover all of the missing amount, the pallet does not continue processing this transfer but emits a TransferPaused event instead.

Next, the pallet calls the transfer() function of the Tokens pallet to transfer the available EURC on the pallet account to the offramping account and emits a TransferProcessed event. !TBD! this might have to change depending on the EUR issuer

Continuing from paused transfers

This is not part of this ticket but is covered in #378.

ebma commented 1 year ago

@pendulum-chain/product this ticket is required for the EuroBrasil project. It's about adding the pallet that automates the currency conversion and offramping. Please assign a high priority to it.

TorstenStueber commented 1 year ago

I assume that the extrinsics are root extrinsics?

We still need to address one important issue, namely the guarantee of the exchange rate. You provided the idea of a "buffer pool" that can provide a missing amount or that will absorb surplus tokens from the swap.

However, this means that the exchange rate (or target amount) displayed to the user on the UI needs to be available on chain. How can we do this? We could for example have an authorized account that would store this information.

However, this doesn't sound elegant to me and I am worried that it can be exploited.

Another option is to somehow encode this also in the reference, e.g., let the reference be a concatenation of the Mykobo reference and the target amount (or exchange rate). But this can also clearly be exploitet and the sender could spoof an arbitrary exchange rate.

Any other ideas?

prayagd commented 1 year ago

Hey team! Please add your planning poker estimate with Zenhub @b-yap @ebma @gianfra-t @TorstenStueber @bogdanS98

ebma commented 1 year ago

Thanks for the comment @TorstenStueber. I favor the former of the approaches you mentioned and I changed the description of this ticket as to how I imagine this to work. I introduced an extra account for the buffer pool. In theory, we don't even need to have two accounts and we could just have one pallet account that we charge with some EURC tokens which would then handle the buffering, but I think it's cleaner to separate these two concerns.

About your concern about this authorized account setup being exploited, I think you are right that if someone gains access to one of the authorized accounts, he can deplete the EURC tokens stored in the buffer pool account. But we can control the severity of this risk by monitoring the balance of that account and only charging it with amounts that we are comfortable losing.

Encoding the amount or exchange rate in the request wouldn't work as you mentioned and I don't have better ideas so far.

TorstenStueber commented 1 year ago

Yeah, I thought about it and I think it is okay and natural that we control this "buffer account". At the end Pendulum as a blockchain is the neutral system and then Pendulum Pay is a service built on top of that that we provide (whereas "we" is in that sense a separate entity, e.g., SatoshiPay) and in order to provide a smooth service we control this account.

A few more thoughts I have:

gianfra-t commented 1 year ago

For the remark issue, it is true that it does not really perform any state change. But as long as the batch call from this pallet is registered as it would be with any extrinsic, then the indexer should pick it up and append the data.

I guess the true question is if it indeed gets registered as a call. Is this what you are referring to @TorstenStueber?

ebma commented 1 year ago

They could call addEurAmountForReference and empty the buffer account this way. However, this would not be freely transferable but would go through a offramping with Mykobo for a KYC'd account so that I think that the danger is limited.

True, that's also what I thought of.

For now we could of course just not complete the transaction and leave the funds on the pallet account. That might be enough for the MVP (what do you say @vadaynujra?)

I would try to cover as much as possible with the funds in the buffer account and execute the off-ramp with that. IMO not completing the transaction is not an option because this means that the user essentially loses all of his on-ramped BRZ. At least if we don't implement a fail-safe mechanism that somehow transfers it back but this is too complex for now.

We need to issue the addEurAmountForReference transaction as soon as we display the QR code to the user. Once the funds are onramped we would expect that there is already an entry in the ReferenceToEurAmount. The timing here will most likely work but we again need to define what happens if such an entry is not yet in the storage. Again, for the MVP it could be enough to just not complete the transaction.

For this, when writing the spec in the description I also assumed that we would just not execute the off-ramp. But now that I think of it, considering my comment on the last question, we should probably execute the off-ramp with the EURC amount swapped with Nabla, not interacting with the buffer pool account. Otherwise, we again face a situation where the user loses all his BRZ or we have to find a way to give it back to them.

Usually the offramp would just be a batch transaction consisting of a transfer and a remark. This way the indexer can easily report that a certain transfer is associated with a specific reference. I don't think that this would work here. The remark intrinsic is actually not doing anything and the whole purpose is that by calling this extrinsic as a transaction, the remark would just be stored in the transaction set of the block. In our case the remark would not be stored at all. So I think we need to find a different solution.

Oh nooooo, I didn't think of that. You are right, we only have access to the remark when looking at the call contained in a transaction. But we are probably not creating a call here. What we could do is, use the remark_with_event extrinsic instead and then storing the hash value in our indexer. And then Mykobo would need to also store the hash of each off-ramp reference number in their database and check the transfers reported by the indexer against that hash. This could work, no?

I'm not sure though if calling the remark() extrinsic from our pallet would result in a call or not. We should probably test this first so we are 100% sure, before changing the implementation again. Or does anyone know @pendulum-chain/devs?

TorstenStueber commented 12 months ago

@ebma In principle I agree with your second and third comment (shall we continue with the offramp if the buffering does not work as expected?)

However, this should also be a dedicated product decision. @vadaynujra @prayagd opinions?

@ebma @gianfra-t about remarks and calls. What even is a call here? If we consider a call to be any of the extrinsics stored (and executed) in a block, then we would not generate a call. We could generate a call by explicitly submitting an extrinsic to the transaction pool – this is basically what offchain workers do. But I don't think that this is the right approach and this would only work for unsigned extrinsics (for offchain workers it also works for signed extrinsics but the offchain worker would then use the secret key stored in the collator node authoring the block).

The remark_with_event would at least leave a trace in the log section of the block. However, it would then also be unclear how that is connected to a specific token transfer happening in that block. We could introduce assumptions like at most one EuroBrasil project related transaction per block but I am not a fan of this.

We could introduce a new event in the automation pallet that contains all the important pieces of information and just emit that event. Then our indexer would allow to watch these events.

ebma commented 12 months ago

We could introduce a new event in the automation pallet that contains all the important pieces of information and just emit that event. Then our indexer would allow to watch these events.

Right, I guess this is the best option we have.

shall we continue with the offramp if the buffering does not work as expected?

Regarding this, I think if we don't want to continue without buffering and thus not achieving the designated exchange rate, we could alternatively stop processing the request in the pallet and wait until the buffer pool is charged again. We could add an extra extrinsic like continueOfframpForReference(reference) callable by anyone, which would retry the processing of the offramp requests, re-checking if the buffer pool account holds enough tokens this time. This approach means that the execution of offramp requests does not always happen immediately and we'd need to add some kind of monitoring, but at least we could then again guarantee that the targeted exchange rate is achieved.

gianfra-t commented 12 months ago

We could introduce a new event in the automation pallet that contains all the important pieces of information and just emit that event. Then our indexer would allow to watch these events.

I think this is better, we are already creating a new pallet so it is not so much extra modifications. I also cannot think of how we could do this with the remark_with_event since we won't have any identification of the transaction internally.

TorstenStueber commented 11 months ago

Okay, then let's add the new event.

For @ebma's point (introduce continueOfframpForReference(reference)): I think this makes a lot of sense. For every swap on Nabla we can state what minimum amount we want to receive in return. This is what we should use to ensure that the exchange rate is not too bad. Then there are two "breaking points":

If this is not too complex to implement, then I think it is worthy to do this already for the MVP. Hard to tell as we need to be very careful not to introduce vulnerabilities. Let's split it into an extra ticket, though and then estimate it separately.

ebma commented 11 months ago

I updated the description. It now mentions two events and the 'continuation' of paused transfers is covered in #378.

prayagd commented 11 months ago

@ebma Thank you, expecting this is ready to work moving it to the ready column

ebma commented 11 months ago

I changed the description slightly. I think we should still take the assumption that we will have one reference number that uniquely identifies each forex transfer and a designated off-ramp account on-chain. But we'll only know once we find another issuer for the EUR asset.

TorstenStueber commented 11 months ago

@annatekl @prayagd I think that we can move this back to Ready now as @ebma adapted the description.