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

Unwrap NEAR on Aurora->NEAR transfers #750

Merged
merged 36 commits into from
Sep 22, 2023
Merged

Unwrap NEAR on Aurora->NEAR transfers #750

merged 36 commits into from
Sep 22, 2023

Conversation

karim-en
Copy link
Contributor

@karim-en karim-en commented May 4, 2023

Description

NEAR instead of wNEAR on AuroraStoring native NEAR tokens on aurora account on NEAR - issues:

  • This creates issues for accounting and monitoring - need to verify which part of aurora balance is used for storage, which is used for bridged NEAR, and which is free and available.
  • Could create a lack of NEAR collateral for the bridged wNEAR - some users may not be able to withdraw their wNEAR from Aurora.

The solution:

  • Keep holding wNEAR on aurora balance on NEAR side.
    • This way, each time the user bridges NEAR to Aurora, it's being wrapped to wNEAR and sent to Aurora.
    • We preserve the same ERC-20 address of wNEAR on Aurora without any additional steps.
  • When withdrawing wNEAR from Aurora, make unwrap in wNEAR (call wrap.near near_withdraw()) and then send native NEAR tokens for users.
    • Use optional suffix {}:unwrap so it won't break the cross-chain swaps for Ref.Finance.

Implementation:
Modify the ExitToNear precompile:

  • Call near_withdraw instead of ft_transfer if the erc20_address is a wnear_address and the recipient end with the suffix {}:unwrap.
  • Rename refund_on_error callback to exit_to_near_precompile_callback.
  • Pass to it args with optional TransferNearCallArgs and an already existing RefundCallArgs but make it optional.
  • In the exit_to_near_precompile_callback transfer near on successful promise result and refund on the failed result.

Performance / NEAR gas cost considerations

The gas consumption increased a little bit due to a callback call.

Testing

Integration tests are added

How should this be reviewed

The reviewer should focus on the changed components that are described in the implementation section, and verify that the implementation is backward compatible and doesn't break any other components like XCC and refund logic.

@sept-en sept-en self-requested a review May 5, 2023 01:50
@joshuajbouw
Copy link
Contributor

Why treat WNEAR differently than every other ERC-20 bridged token?

@sept-en
Copy link
Contributor

sept-en commented May 11, 2023

@joshuajbouw this is because $NEAR is a native token in NEAR, and not NEP-141. So this change could allow users to create and/or top-up NEAR accounts easier and in more convenient way (it doesn't require a separate NEAR transaction and thus NEAR gas to unwrap wNEAR from the user).

Also, on NEAR->Aurora transfers of $NEAR we automatically wrap them so users receive wNEAR on Aurora. This PR will allow unwrapping on the way back.

@mrLSD mrLSD self-requested a review May 11, 2023 13:08
@karim-en karim-en marked this pull request as ready for review May 19, 2023 12:26
@karim-en karim-en requested a review from vimpunk as a code owner May 19, 2023 12:26
@joshuajbouw
Copy link
Contributor

Looks like CI is still failing.

@joshuajbouw
Copy link
Contributor

Can you resolve conflicts please? Thanks.

Copy link
Member

@birchmd birchmd left a comment

Choose a reason for hiding this comment

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

PR looks good except for one mistake in the TransactionKindTag

engine-standalone-storage/src/sync/types.rs Show resolved Hide resolved
@aleksuss aleksuss added this pull request to the merge queue Sep 22, 2023
Merged via the queue into develop with commit 00f41ee Sep 22, 2023
20 checks passed
@aleksuss aleksuss deleted the unwrap-wnear branch September 22, 2023 15:40
aleksuss added a commit that referenced this pull request Sep 25, 2023
<!--
Thanks for submitting a pull request! Here are some helpful tips:

* Always create branches on and target the `develop` branch.
* Run all the tests locally and ensure that they are passing.
* Run `make format` to ensure that the code is formatted.
* Run `make check` to ensure that all checks passed successfully.
* Small commits and contributions that attempt one single goal is
preferable.
* If the idea changes or adds anything functional which will affect
users, an
AIP discussion is required first on the Aurora forum: 

https://forum.aurora.dev/discussions/AIPs%20(Aurora%20Improvement%20Proposals).
* Avoid breaking the public API (namely in engine/src/lib.rs) unless
required.
* If your PR is a WIP, ensure that you enable "draft" mode.
* Your first PRs won't use the CI automatically unless a maintainer
starts.
If this is not your first PR, please do NOT abuse the CI resources.

Checklist:
- [ ] I have performed a self-review of my code
- [ ] I have documented my code, particularly in the hard-to-understand
areas
- [ ] I have made corresponding changes to the documentation
- [ ] I have added tests to prove my fix or new feature is effective and
works
- [ ] Any dependent changes have been merged
- [ ] The PR is targeting the `develop` branch and not `master`
- [ ] I have pre-squashed my commits into a single commit and rebased.
-->

## Description

NEAR instead of wNEAR on AuroraStoring native NEAR tokens on aurora
account on NEAR - issues:
* This creates issues for accounting and monitoring - need to verify
which part of `aurora` balance is used for storage, which is used for
bridged NEAR, and which is free and available.
* Could create a lack of NEAR collateral for the bridged wNEAR - some
users may not be able to withdraw their wNEAR from Aurora.

The solution:
* Keep holding wNEAR on `aurora` balance on NEAR side.
* This way, each time the user bridges NEAR to Aurora, it's being
wrapped to wNEAR and sent to Aurora.
* We preserve the same ERC-20 address of wNEAR on Aurora without any
additional steps.
* When withdrawing wNEAR from Aurora, make unwrap in wNEAR (call
`wrap.near near_withdraw()`) and then send native NEAR tokens for users.
* Use optional suffix `{}:unwrap` so it won't break the cross-chain
swaps for [Ref.Finance](http://ref.finance/).

Implementation:
Modify the `ExitToNear` precompile:
* Call `near_withdraw` instead of `ft_transfer` if the `erc20_address`
is a `wnear_address` and the `recipient` end with the suffix
`{}:unwrap`.
* Rename `refund_on_error` callback to
`exit_to_near_precompile_callback`.
* Pass to it args with optional `TransferNearCallArgs` and an already
existing `RefundCallArgs` but make it optional.
* In the `exit_to_near_precompile_callback` transfer near on successful
promise result and refund on the failed result.

## Performance / NEAR gas cost considerations
The gas consumption increased a little bit due to a callback call.
<!--
Performance regressions are not ideal, though we welcome performance 
improvements. Any PR must be completely mindful of any gas cost
increases. The
CI will fail if the gas costs change at all. Do update these tests to 
accommodate for the new gas changes. It is good to explain 
this change, if necessary.
-->

## Testing
Integration tests are added
<!--
Please describe the tests that you ran to verify your changes.
-->

## How should this be reviewed
The reviewer should focus on the changed components that are described
in the `implementation` section, and verify that the implementation is
backward compatible and doesn't break any other components like `XCC`
and `refund` logic.

---------

Co-authored-by: Oleksandr Anyshchenko <oleksandr.anyshchenko@aurora.dev>
Co-authored-by: Michael Birch <michael.birch@aurora.dev>
@aleksuss aleksuss mentioned this pull request Sep 25, 2023
aleksuss added a commit that referenced this pull request Sep 25, 2023
- Added the possibility to use native NEAR instead of wNEAR on Aurora by
[@karim-en]. ([#750])

- Added hashchain integration by [@birchmd]. ([#831])

- Added functions for setting and getting metadata of ERC-20 contracts
deployed with `deploy_erc20_token` transaction
  by [@aleksuss]. ([#837])

---------

Co-authored-by: Michael Birch <michael.birch@aurora.dev>
Co-authored-by: Evgeny Ukhanov <evgeny@aurora.dev>
Co-authored-by: Karim <karim@aurora.dev>
github-merge-queue bot pushed a commit that referenced this pull request Nov 17, 2023
## Description

Since #750 has been merged, the ERC-20 connector is able to unwrap
wrapped Near tokens into Near native tokens. This logic was duplicated
by the XCC router, but in this PR we remove the logic from XCC and use
the bridge's unwrapping instead. This has a nice side-effect of saving
us 20 Tgas from the XCC overhead.

## Performance / NEAR gas cost considerations

No impact on regular transactions, 20 Tgas improvement to XCC
transactions.

## Testing

Existing tests. Specifically this relies heavily on the upgrade test
introduced in #866
aleksuss pushed a commit that referenced this pull request Nov 28, 2023
## Description

Since #750 has been merged, the ERC-20 connector is able to unwrap
wrapped Near tokens into Near native tokens. This logic was duplicated
by the XCC router, but in this PR we remove the logic from XCC and use
the bridge's unwrapping instead. This has a nice side-effect of saving
us 20 Tgas from the XCC overhead.

## Performance / NEAR gas cost considerations

No impact on regular transactions, 20 Tgas improvement to XCC
transactions.

## Testing

Existing tests. Specifically this relies heavily on the upgrade test
introduced in #866
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

Successfully merging this pull request may close these issues.

None yet

7 participants