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

Feat: allow borrow automation #1733

Merged
merged 9 commits into from
Feb 27, 2024
Merged

Conversation

mustermeiszer
Copy link
Collaborator

@mustermeiszer mustermeiszer commented Feb 16, 2024

Description

Currently, we are on/off-ramping tokens from/to AssetHub frequently. Unfortunately, Circle does not recognize token transfers made through XCM. I.e. tokens are lost, because Circle does not know which account send them. My best guess would be, that the internal XCM transfer logic, using only traits does not trigger a single event that Circle can rely on.

But with the new remote account derivation logic that was introduces to AssetHub, we can fix this by doing a Transact(pallet_assets::transfer(..)) from the users remote account.

  • Spec in detail is here - this PR uses Option A.

The proposed solution in our case here will look like:

  • Deposit into Circle: batch_all(xtokens::transfer(..), palletXcm::send(..)).
    The first extrinsic will send a transfer tokens XCM, that will deposit the tokens in the remote account of the user, the second extrinsic will call Transact(pallet_assets::transfer(.., dest: Circle)) with the remote account of the user as the origin.
  • Withdraw from Circle: batch_all(palletXcm::send(..)).
    • The extrinsic will call Transact(palletXcm::transfer(.., dest: Centrifuge)) with the remote account of the user as the origin. Sending the already existing tokens from the users account on AssetHub via XCM to Centrifuge*

Changes and Descriptions

  • Allow dispatch of pallet_xcm::send(..) by any Origin::signed(..) - NOTE: No harm here, as we are not executing that on our chain. Messages is send straigt to destination
  • LocalOriginToLocation fix - uses correct NetworkId now

Tasks

  • Implement above changes
  • Write integration tests for our AccountConverter - The thing is too important and we should have some hardcoded derivations to know that it is not changing might flieght
  • Test with choopsticks and live state from Polkadot and Centrifuge

Checklist:

  • I have added Rust doc comments to structs, enums, traits and functions
  • I have made corresponding changes to the documentation
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works

@mustermeiszer mustermeiszer added D0-ready Pull request can be merged without special precaution and notification. D2-notify Pull request can be merged and notification about changes should be documented. and removed D0-ready Pull request can be merged without special precaution and notification. labels Feb 16, 2024
@mustermeiszer
Copy link
Collaborator Author

@onnovisser and @annamehr this PR will allow to make on/off-ramping mostly automatic from the UI side. Which XCM to actually generate in the end I will post here and we can test it then in the UI. Unfortunately, I think we need to test the UI in the production network. But lets see.

@wischli
Copy link
Contributor

wischli commented Feb 16, 2024

But with the new remote account derivation logic that was introduces to AssetHub, we can fix this by doing a Transact(pallet_assets::transfer(..)) from the users remote account.

  • Spec in detail is here - this PR uses Option A.

Another reason to upgrade to Polkadot v1.1.0 ASAP because that's the earliest version which supports remote account derivation AFAICS.

@mustermeiszer
Copy link
Collaborator Author

Yes, but we already got that in with out own custom deriation. So there is no need for that to enable that feature, but for sure polkadot upgrade is well needed now.

@mustermeiszer
Copy link
Collaborator Author

@wischli I would merge this one now that you have confirmed that it is working. The fees and all around that are AH related.

@mustermeiszer mustermeiszer marked this pull request as ready for review February 27, 2024 08:41
@@ -93,7 +93,7 @@ impl<T: Runtime> Env<T> for RuntimeEnv<T> {
call: impl Into<T::RuntimeCallExt>,
) -> Result<Balance, DispatchError> {
let call: T::RuntimeCallExt = call.into();
let info = call.get_dispatch_info();
let info = self.parachain_state(|| call.get_dispatch_info());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT. Maybe can be moved directly inside line 126

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like info is used only in line 126 under an already parachain_state(). So maybe call.get_dispatch_info() can be moved there. But a super NIT thing.

@wischli wischli added this to the Centrifuge 1025 milestone Feb 27, 2024
@mustermeiszer mustermeiszer enabled auto-merge (squash) February 27, 2024 09:39
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering whether we want to add RtApi calls to generate the derived accounts depending on the remote location. If at all, probably something for a subsequent PR. Right now, AH is the only expected remote location such that Apps could hardcode the derivation.

@mustermeiszer mustermeiszer merged commit 79c0839 into main Feb 27, 2024
9 checks passed
@mustermeiszer
Copy link
Collaborator Author

Already possible with the conversion_of api

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D2-notify Pull request can be merged and notification about changes should be documented.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants