Feat(examples): FT Refund #13

birchmd · 2023-08-22T14:19:42Z

This example shows how to bridge a NEP-141 token from Aurora to Near to use in a contract there and includes a refund flow where if the token is refunded to the XCC account on Near then it is automatically bridged back to Aurora and credited to the original transaction signer's account.

See the README in the example for more information.

mooori · 2023-08-23T14:05:37Z

cargo test in examples/ft-refund fails with:

Compiling ft-refund v0.1.0 (/home/mo/opensource/aurora-contracts-sdk/examples/ft-refund/near-contract)
error[E0599]: no method named `verbose_call_evm_contract_with` found for reference `&AuroraEngine` in the current scope
   --> integration-tests/src/lib.rs:313:18
    |
313 |                 .verbose_call_evm_contract_with(
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: there is a method with a similar name: `call_evm_contract`

For more information about this error, try `rustc --explain E0599`.
error: could not compile `recursive-callbacks-integration-tests` due to previous error

AuroraEngine has a method call_evm_contract_with (missing verbose_) which seems to have the same signature. Are these two methods equivalent apart from different log levels?

birchmd · 2023-08-23T14:08:38Z

Thanks @mooori Sorry about that. That was left over from some debugging. Fixed in 6b44501

sept-en · 2023-08-23T14:16:16Z

Requested @karim-en and @olga24912 reviews as this seems to be at least partly common with the functionality implemented in Near-One/rainbow-token-connector#225

olga24912 · 2023-08-24T09:11:29Z

examples/ft-refund/solidity-contract/src/FtRefund.sol

+    constructor(string memory _nearAccountId, IERC20 _wNEAR) {
+        nearAccountId = _nearAccountId;
+        near = AuroraSdk.initNear(_wNEAR);
+        wNEAR = _wNEAR;


Why we save wNEAR as well and not just use near.wNEAR?

The fields of the NEAR struct are not public, so cannot be used outside the SDK itself. Being the maintainers of the SDK that is something we have the power to change, but I'm not sure if we want to. Exposing the fields publicly leaks some of the implementation details and we may want to change them in the future.

olga24912 · 2023-08-24T09:32:36Z

examples/ft-refund/solidity-contract/src/FtRefund.sol

+        );
+    }
+
+    function approveWNEAR() public {


If I correctly understood the meaning of this function -- approve will be called by CALLBACK_ROLE and we will allowe this contract to spend the CALLBACK_ROLE's wNEAR. This should be explained in the comments to this function. And who provided wNEAR to the CALLBACK_ROLE?

This is the tricky part of this example (and a sort of pitfall in the XCC design). The CALLBACK_ROLE is logically the same as this contract because it is the implicit address of this contract's XCC account (and therefore all transactions from CALLBACK_ROLE actually originated from this contract). So this function is essentially just approving itself to spend its own tokens. The reason its needed is because the addresses are different on the level of the EVM even though they are logically the same entity as I have described above. I tried to explain this in the README file and comments in the integration tests. But I could add a comment here too.

I think it is the maintainer of this contract who is responsible for funding WNEAR to CALLBACK_ROLE. It only requires 1 yoctoNEAR per call, so if a user were to try to maliciously drain the WNEAR by intentionally making failed calls they would need to invest a lot of ETH or NEAR in gas fees. If the maintainer initially funded it with 1 WNEAR then 10^24 refunds would be needed to drain it and that is infeasible.

I think a comment referring to the README/docs would be helpful. As mentioned, it's the tricky part and someone who's using this example to learn might not get it right away.

Added a comment in 6180a22

olga24912 · 2023-08-24T09:34:37Z

examples/ft-refund/solidity-contract/src/FtRefund.sol

+                1,
+                REFUND_NEAR_GAS
+            );
+            callFtTransfer.transact();


What if this function will fail? For example because we don't have 1 yoctowNEAR.

If this function fails then the funds would remain in the contract's XCC account. It would be possible to add an owner-only manual recovery as a fallback option.

mooori

To help myself with the review, I’ve made the diagram below. I’m not quite sure if I understood everything that’s going on correctly. @birchmd could you please have a look and let me know if the diagram is accurate? If yes, adding it to the README perhaps could help other readers too.

examples/ft-refund/integration-tests/src/lib.rs

examples/ft-refund/README.md

examples/ft-refund/near-contract/src/lib.rs

examples/ft-refund/solidity-contract/src/IEvmErc20.sol

mooori · 2023-08-28T09:08:50Z

examples/ft-refund/solidity-contract/src/FtRefund.sol

+        );
+    }
+
+    function approveWNEAR() public {


I think a comment referring to the README/docs would be helpful. As mentioned, it's the tricky part and someone who's using this example to learn might not get it right away.

examples/ft-refund/solidity-contract/src/FtRefund.sol

mooori

@birchmd thanks for the clarifications and updates! I’ve proposed some comments to make assumptions and requirements more explicit. If they are needed, I suppose, depends on the expected audience and their familiarity with token bridging and Aurora engine internals. Feel free to modify or discard these comments.

The commit to add the diagram to the README (as discussed offline) is coming up shortly.

examples/ft-refund/solidity-contract/src/FtRefund.sol

examples/ft-refund/integration-tests/src/lib.rs

mooori

LGTM. One note though: When commenting out this call

aurora-contracts-sdk/examples/ft-refund/integration-tests/src/lib.rs

Lines 221 to 229 in 5752b12

    
           solidity_contract 
        
               .ft_transfer_call( 
        
                   &account, 
        
                   &test_erc20.address, 
        
                   test_token_contract.id().to_string(), 
        
                   1, 
        
               ) 
        
               .await 
        
               .unwrap();

the test still passes, as there seem to be no checks of intermediate states (which are quite a lot for this example).

I’m not sure if this is relevant for an example. Anyway, one way to tackle this might be making near-contract keep a small amount if the msg is "refund”, e.g. as a fee. Then the following assert could check the balance is amount - fee to ensure something has happened.

aurora-contracts-sdk/examples/ft-refund/integration-tests/src/lib.rs

Lines 232 to 233 in 5752b12

    
           let erc20_balance = engine.erc20_balance_of(&test_erc20, address).await.unwrap(); 
        
           assert_eq!(erc20_balance, "1".into());

karim-en · 2023-09-19T10:30:50Z

examples/ft-refund/solidity-contract/src/FtRefund.sol

+        uint128 amount
+    ) public {
+        token.transferFrom(msg.sender, address(this), amount);
+        token.withdrawToNear(


@birchmd In our fast bridge and silo to silo transfers contracts we added this comment.

// WARNING: The withdrawToNear() method works asynchronously.
// As a result, there is no guarantee that this method will be completed before ft_transfer_call().
// In case of such an error, the user will be able to call the withdraw() method and get his tokens back.
// We expect such an error not to happen as long as transactions are executed in one shard.

Do you agree with these conclusions?

No, I do not think that is possible with this example (I'm not sure if the bridge contracts differ from this example significantly). The reason is because the important receipts are going through the same NEP-141 contract and I think the protocol implementation guarantees as FIFO ordering on receipts without data dependencies (at least when talking about cross-shard receipts; local receipts are always processed first).

What happens in this example is that withdrawToNear creates an ft_transfer receipt to the NEP-141 contract (transferring the tokens from Aurora to the user's XCC account) and at the same time the XCC mechanism creates a receipt calling the execute method on the user's XCC account. When the execute receipt resolves it will create a new receipt calling the ft_transfer_call function on the NEP-141 contract. No matter what, this second receipt is created after the first one created by Aurora. Even if the first receipt does not resolve right away (because of congestion on the NEP-141 contract's shard), the FIFO execution will ensure the ft_transfer (i.e. withdrawToNear) executes before ft_transfer_call.

Turns out what I thought was true about FIFO ordering was incorrect. The bridge did in fact encounter an error due to receipts being executed out of order. So the warning in the comment is accurate.

Fortunately the issue should come up rarely because there is still 1 block between when the ft_transfer receipt is created and when the ft_transfer_call receipt is. So the only way an error could happen is if there is congestion on the shard containing the NEP-141 token causing the ft_transfer receipt to be delayed by one or more blocks.

This issue is addressed at the Engine level in aurora-is-near/aurora-engine#868

birchmd · 2023-11-09T16:00:02Z

making near-contract keep a small amount if the msg is "refund”, e.g. as a fee. Then the following assert could check the balance is amount - fee to ensure something has happened.

Thanks for the suggestion @mooori ! Implemented in 10ee691

Review comments have been addressed

…as amounts

Co-authored-by: Moritz Zielke <[email protected]>

…a small fee on refund

@karim-en

## Description This PR is addressing a [usability issue with XCC](aurora-is-near/aurora-contracts-sdk#13 (comment)) brought up by @karim-en . It is somewhat common that XCC requires tokens to be bridged from the user's Aurora address to their XCC account on Near. Naturally, this bridging needs to happen before the XCC promise resolves so that the tokens are available for it to use. However, due to the async nature of Near we could not guarantee that the bridging would happen before the XCC call. In this PR we change how the Engine produces promises so that they are sequential instead of concurrent (i.e. all the promises produced by the EVM are connected by the `then` combinator, in the order they were produced during the EVM execution). This means a contract which calls the exit precompile and then later calls XCC will have the promises happen in that same order, as the developer intended. ## Performance / NEAR gas cost considerations N/A ## Testing Existing XCC tests

@karim-en

## Description This PR is addressing a [usability issue with XCC](aurora-is-near/aurora-contracts-sdk#13 (comment)) brought up by @karim-en . It is somewhat common that XCC requires tokens to be bridged from the user's Aurora address to their XCC account on Near. Naturally, this bridging needs to happen before the XCC promise resolves so that the tokens are available for it to use. However, due to the async nature of Near we could not guarantee that the bridging would happen before the XCC call. In this PR we change how the Engine produces promises so that they are sequential instead of concurrent (i.e. all the promises produced by the EVM are connected by the `then` combinator, in the order they were produced during the EVM execution). This means a contract which calls the exit precompile and then later calls XCC will have the promises happen in that same order, as the developer intended. ## Performance / NEAR gas cost considerations N/A ## Testing Existing XCC tests

birchmd requested a review from mooori August 22, 2023 14:19

sept-en requested review from sept-en, olga24912 and karim-en August 23, 2023 14:13

sept-en assigned sept-en, olga24912 and karim-en Aug 23, 2023

olga24912 previously requested changes Aug 24, 2023

View reviewed changes

mooori reviewed Aug 29, 2023

View reviewed changes

mooori reviewed Sep 1, 2023

View reviewed changes

examples/ft-refund/solidity-contract/src/FtRefund.sol Show resolved Hide resolved

mooori reviewed Sep 1, 2023

View reviewed changes

examples/ft-refund/solidity-contract/src/FtRefund.sol Outdated Show resolved Hide resolved

examples/ft-refund/solidity-contract/src/FtRefund.sol Outdated Show resolved Hide resolved

examples/ft-refund/integration-tests/src/lib.rs Show resolved Hide resolved

mooori approved these changes Sep 4, 2023

View reviewed changes

karim-en reviewed Sep 19, 2023

View reviewed changes

karim-en mentioned this pull request Sep 21, 2023

AuroraToEth Fast Bridge: Ether transfer Near-One/fast-bridge-protocol#113

Merged

birchmd requested a review from olga24912 November 9, 2023 16:05

Tarnadas and others added 10 commits November 9, 2023 19:05

feat: ft refund

6188e54

Ensure xcc representative address is allowed to spend wnear, modify g…

b8e93d7

…as amounts

Minor clean-up, more explanation

2be6bf9

Remove verbose call

5ba4e69

Do not specify Rust contract in README

cfc758b

Fix typo in README

386f045

Add comment describing the purpose of black_hole function

fa7ea05

Add comment on approveWNEAR Solidity function

8174c79

Add comment on ftTransferCall Solidity function

b931e3a

Add overview diagram to README

44232c0

mooori and others added 5 commits November 9, 2023 19:05

Fix diagram path

ad244ef

Fix mistake in comment

e3a7e5f

Add note about approval to comment

3ac5d96

Co-authored-by: Moritz Zielke <[email protected]>

Add note about how bridging works to comment

bb3ebcd

Co-authored-by: Moritz Zielke <[email protected]>

Factor out minted amount of tokens as a constant; have contract take …

33a8898

…a small fee on refund

birchmd force-pushed the ft-refund branch from 6cf2928 to 33a8898 Compare November 9, 2023 18:09

birchmd merged commit 76cb2f4 into main Nov 9, 2023

birchmd deleted the ft-refund branch November 9, 2023 18:10

This was referenced Nov 17, 2023

Feat: Exit and XCC promises are sequential aurora-is-near/aurora-engine#868

Merged

sending tokens from Aurora to Near #10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat(examples): FT Refund #13

Feat(examples): FT Refund #13

birchmd commented Aug 22, 2023

mooori commented Aug 23, 2023

birchmd commented Aug 23, 2023

sept-en commented Aug 23, 2023

olga24912 Aug 24, 2023

birchmd Aug 24, 2023

olga24912 Aug 24, 2023

birchmd Aug 24, 2023

mooori Aug 28, 2023

birchmd Aug 30, 2023

olga24912 Aug 24, 2023

birchmd Aug 24, 2023

mooori left a comment

mooori Aug 28, 2023

mooori left a comment

mooori left a comment

karim-en Sep 19, 2023

birchmd Sep 19, 2023

birchmd Nov 9, 2023

birchmd Nov 17, 2023

birchmd commented Nov 9, 2023

	solidity_contract
	.ft_transfer_call(
	&account,
	&test_erc20.address,
	test_token_contract.id().to_string(),
	1,
	)
	.await
	.unwrap();

	let erc20_balance = engine.erc20_balance_of(&test_erc20, address).await.unwrap();
	assert_eq!(erc20_balance, "1".into());

Feat(examples): FT Refund #13

Feat(examples): FT Refund #13

Conversation

birchmd commented Aug 22, 2023

mooori commented Aug 23, 2023

birchmd commented Aug 23, 2023

sept-en commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mooori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mooori left a comment

Choose a reason for hiding this comment

mooori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

birchmd commented Nov 9, 2023