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

Create automation pallet for EuroBrasil project #357

Open
ebma opened this issue Nov 29, 2023 · 15 comments
Open

Create automation pallet for EuroBrasil project #357

ebma opened this issue Nov 29, 2023 · 15 comments
Assignees

Comments

@ebma
Copy link
Member

ebma commented Nov 29, 2023

Context

The context can be found in this Notion page.

Pallet configuration

Name

  • forex-automation

Config Items

  • PalletId - a PalletId used to derive the on-chain accounts for the automation and the buffer pool

Storage Items

  • NablaRouterAddress - the address of Nabla’s router smart contract
  • EurcErc20Address - the address of the ERC-20 wrapper contract for the EURC token
  • BrzErc20Address - the address of the ERC-20 wrapper contract for the BRZ token
  • OfframpAccount - the address of the designated off-ramping account. !TBD! This might not be required based on the EUR issuer
  • ReferenceToEurAmount - a map from Reference -> Balance which is used to store the desired target amount of EUR that were initially shown to the user upon creating a transfer request in PendulumPay. We store the amount and not an exchange rate because this way we don't have to incorporate any (on-ramp or XCM) fees that might have been deducted from the BRZ amount until it arrives on our parachain.
  • AuthorizedAccounts - a list of authorized accounts that are allowed to add entries to the ReferenceToEurAmount map

Extrinsics

  • setNablaRouterAddress(address) - sets the address of the router contract. Root extrinsic
  • setErc20Adresses(eurcAddress, brzAddress) - sets the addresses of the erc-20 contracts. Root extrinsic
  • setOfframpAccount(address) - sets the address of the offramping account. Root extrinsic
  • addEurAmountForReference(reference, amount) - adds an entry to the ReferenceToEurAmount. Only accounts registered in AuthorizedAccounts are allowed to call this extrinsic.
  • addAuthorizedAccount(account) - adds an authorized account to AuthorizedAccounts. Callable from Root and other authorized accounts
  • removeAuthorizedAccount(account) - removes an authorized account from AuthorizedAccounts. Callable from Root and other authorized accounts.
  • chargeBufferPoolAccount(currency) - transfers tokens to the pallet's buffer pool account. This is a mere helper function that makes it easy to charge the pool without having to know the address of the buffer pool. Callable from anyone. We don't need to limit the currencies that can be used with this function, but generally, it would only be used to charge the buffer pool with BRZ or EURC tokens.

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

  • processEurcOfframp(amount, reference) - this function is called by the XCM config of our runtime. It handles the conversion from BRZ → EUR and transfers the tokens to the offramping account. amount is the amount of BRZ tokens that are to be offramped and reference is the reference that was encoded by the MultiLocation
  • getPalletAccount() - this function returns the account of this pallet, derived from the PalletId configuration parameter. This can be done with <T as Config>::PalletId::get().into_sub_account_truncating(0)
  • getBufferPoolAccount() - this function returns the account of the pool buffer, derived from the PalletId configuration parameter. This can be done with <T as Config>::PalletId::get().into_sub_account_truncating(1)

Events

  • TransferProcessed(reference, amount, currency) - emitted after sending the swapped tokens to the designated offramping account.
  • TransferPaused(reference, amount, currency) - emitted when the offramp cannot be executed with the targeted amount. This happens if the buffer pool does not have enough funds to cover the rest or if a Nabla swap is too expensive.

Coupling to other pallets

  • Contracts - needed to call the nabla contracts
  • Tokens - needed for the transfer
  • System - needed to set the remark

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
Copy link
Member Author

ebma commented Nov 29, 2023

@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
Copy link
Member

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
Copy link
Collaborator

prayagd commented Nov 30, 2023

@ebma
Copy link
Member Author

ebma commented Nov 30, 2023

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
Copy link
Member

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:

  • What is the potential attack vector someone has if they get access to one of the authorized accounts? 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.

  • What happens if the buffer account is not covered sufficiently (let's say some attack on Nabla leads to heavy slippage and therefore the exchange rate is much worse than expected). We would need to define what happens in this situation. 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?)

  • 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.

  • 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.

@gianfra-t
Copy link
Contributor

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
Copy link
Member Author

ebma commented Dec 1, 2023

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
Copy link
Member

@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
Copy link
Member Author

ebma commented Dec 6, 2023

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
Copy link
Contributor

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
Copy link
Member

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":

  • Nabla cannot give us the minimum exchange rate we need or
  • Even if the Nabla swap works there are not enough funds in the buffer account to pay the difference.

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
Copy link
Member Author

ebma commented Dec 12, 2023

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

@prayagd
Copy link
Collaborator

prayagd commented Dec 14, 2023

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

@ebma
Copy link
Member Author

ebma commented Dec 21, 2023

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
Copy link
Member

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

@gianfra-t gianfra-t self-assigned this Feb 2, 2024
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

When branches are created from issues, their pull requests are automatically linked.

4 participants