Switch to FT-141 for BridgeToken #39

ilblackdragon · 2021-03-04T14:10:33Z

Implements basics of FT-141.
Sim tests are still not finished and everything should be looked over again to see if there are better flows (like react on receiving to factory_id for withdrawing tokens)

…Token

bridge-token/src/lib.rs

ilblackdragon · 2021-03-04T14:27:50Z

Filled the bug found in near-sdk-rs: near/near-sdk-rs#306

alexauroradev · 2021-03-04T22:10:38Z

Fixes #39

bridge-token-factory/src/lib.rs

bridge-token/src/lib.rs

sept-en · 2021-03-05T00:58:50Z

This PR fixes #21.

Leverage rust enum types for better interface of ResultType Formatting

mfornet · 2021-03-05T11:29:03Z

Right now mint always require attached a NEAR deposit, but there are several options:

Do nothing => mint always require attached $N (1 yoctoNear is fine if no registration is needed, but then FullAccessKey signature is required)
Have two functions, one to register (also one to unregister) and one for minting. Only register and unregister are payable, but mint isn't.
Have two mint functions (one that is payable and allows registration, and one that is not payable but require user to be already registered).

If in addition there is a view function that allows UI to know in advance whether the user is registered or not we can improve UX for options 2 and 3. However they still need to first sign tx to register with FullAccessKey, and then add extra key to call non-payable mint function.

mfornet · 2021-03-05T11:46:53Z

bridge-token-factory/src/lib.rs

+            .try_to_vec()
+            .unwrap(),
+        );
+        PromiseOrValue::Value(amount)


Should this be 0 given that we are locking all those tokens on this contract?

You are right, should be 0

ilblackdragon · 2021-03-05T12:18:09Z

Is payable actually requires that at least 1 yN gets attached?

Have separate functions

There is a pure storage registration function already in the token, but it's more of a question if user or relayer needs to call it or can just call mint once.
Also note, need to get this PR into near-sdk-rs: near/near-sdk-rs#309 to make sure API is compatible.

ilblackdragon · 2021-03-05T12:20:32Z

The 10x growth is really weird - that means there is something that should be no be compiled in there. Did you try to rebuild wasm file? Also it's weird that cost on testnet is still 10e20, it should be 10e19 now.

…

On Fri, Mar 5, 2021 at 4:17 AM Marcelo Fornet ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In bridge-token-factory/src/lib.rs <#39 (comment)> : > @@ -31,8 +27,6 @@ const BRIDGE_TOKEN_NEW: Gas = 10_000_000_000_000; /// Initial balance for the BridgeToken contract to cover storage and related. const BRIDGE_TOKEN_INIT_BALANCE: Balance = 30_000_000_000_000_000_000_000_000; // 3e25yN, 30N According to my experiment calculations it should be increased 🤔 cost per byte on testnet is: 100000000000000000000 (is the cost already decreased in testnet?) contract size in bytes is: 3769047 (Notice that it is growing in this PR from 481014 to 3769047 almost 10x) BRIDGE_TOKEN_INIT_BALANCE = 30000000000000000000000000 MINIMUM_EXPECTED = 376904700000000000000000000 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#39 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABK27V7F5I6Z37DBS5YUU3TCDDUBANCNFSM4YTKHVAQ> .

-- Best regards, Illia Polosukhin

ilblackdragon · 2021-03-05T12:37:16Z

Cost is reduced by 10x: ``` $ near state dev-1614947681178-6795333 7.3s  Fri Mar 5 04:36:19 2021 Account dev-1614947681178-6795333 { amount: '9999561724097628400000000', locked: '0', code_hash: 'G9KxxEMn6woPq7npJVCbf2s976ptXm6dnH3AMDqVCzCR', storage_usage: 246824, storage_paid_at: 0, block_height: 38933125, block_hash: 'FHTUCmdKLXS9Zf3dGPtV6wk2Vk17MchwfX5ZjEJX1xrz', formattedAmount: '9.9995617240976284' } ``` Here, 9N is enough to cover 246kb. On Fri, Mar 5, 2021 at 4:20 AM Illia Polosukhin <[email protected]> wrote:

…

The 10x growth is really weird - that means there is something that should be no be compiled in there. Did you try to rebuild wasm file? Also it's weird that cost on testnet is still 10e20, it should be 10e19 now. On Fri, Mar 5, 2021 at 4:17 AM Marcelo Fornet ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In bridge-token-factory/src/lib.rs > <#39 (comment)> > : > > > @@ -31,8 +27,6 @@ const BRIDGE_TOKEN_NEW: Gas = 10_000_000_000_000; > > /// Initial balance for the BridgeToken contract to cover storage and related. > > const BRIDGE_TOKEN_INIT_BALANCE: Balance = 30_000_000_000_000_000_000_000_000; // 3e25yN, 30N > > > According to my experiment calculations it should be increased 🤔 > > cost per byte on testnet is: 100000000000000000000 (is the cost already > decreased in testnet?) > contract size in bytes is: 3769047 (Notice that it is growing in this PR > from 481014 to 3769047 almost 10x) > > BRIDGE_TOKEN_INIT_BALANCE = 30000000000000000000000000 > > MINIMUM_EXPECTED = 376904700000000000000000000 > > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#39 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AABK27V7F5I6Z37DBS5YUU3TCDDUBANCNFSM4YTKHVAQ> > . > -- Best regards, Illia Polosukhin

-- Best regards, Illia Polosukhin

mfornet · 2021-03-05T13:15:49Z

@ilblackdragon what contract did you deployed, because in this repository I got this:

❯ ls -l res/bridge_token_factory.wasm
-rwxr-xr-x  1 marcelo  staff  3769047 Mar  5 14:14 res/bridge_token_factory.wasm

This contract is 3769047 bytes long. Is it possible that you deployed old version of the contract? Use latest commit in this PR.

ilblackdragon · 2021-03-08T03:05:54Z

Have you rebuilt it? The one checked in this repo has link symbols and wasn't compiled with all the flags because I was compiling for testing purposes.

…

On Fri, Mar 5, 2021 at 5:16 AM Marcelo Fornet ***@***.***> wrote: @ilblackdragon <https://github.com/ilblackdragon> what contract did you deployed, because in this repository I got this: ❯ ls -l res/bridge_token_factory.wasm -rwxr-xr-x 1 marcelo staff 3769047 Mar 5 14:14 res/bridge_token_factory.wasm This contract is 3769047 bytes long. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#39 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABK27TFUQG2JDX3M24PTZDTCDKRNANCNFSM4YTKHVAQ> .

-- Best regards, Illia Polosukhin

alexauroradev · 2021-03-08T11:41:21Z

Please update to the new interface of Storage Manger: https://github.com/near/near-sdk-rs/blob/master/near-contract-standards/src/storage_manager/mod.rs
Plus, use https://github.com/near/near-sdk-rs/blob/master/near-contract-standards/src/fungible_token/implementation.rs#L56 field to implement storage_minimum_balance

@mikedotexe, the current version of the vanilla token implementation still uses AR instead of a storage manger. Is this something that is going to continue? We need the interface for the storage interaction for mint function

CC: @mfornet

alexauroradev · 2021-03-08T11:42:21Z

Closes #21

alexauroradev · 2021-03-08T13:04:32Z

Also, need to update @mfornet
https://github.com/near/rainbow-token-connector/blob/ft-141/bridge-token-factory/src/lib.rs#L191

alexauroradev · 2021-03-08T13:07:50Z

@mfornet
Need also to add here an assert that the transaction has enough attached deposit to pay for the storage (in case the account is not created). If we're not doing this then the user will lose the proof, if he didn't attach enough deposit (see self.record_proof()).

alexauroradev · 2021-03-09T07:37:35Z

Need also to add here an assert that the transaction has enough attached deposit to pay for the storage (in case the account is not created). If we're not doing this then the user will lose the proof, if he didn't attach enough deposit (see self.record_proof()).

This is much more complicated problem due to the fact of async execution of the contracts, as was mentioned by @abacabadabacaba.

Scenario when the asserts are not working :

Bridge factory checks that there's a user account, so no attached_deposit required
Bridge schedules a minting call to the bridge token
Just before this, a user withdraws all the amount of tokens and deletes the account using storage_unregister here.
Once the initial minting call reaches a factory, there're already no account and no attached_deposit, so the mint fails.

As a result, the proof was consumed, while the transfer was not finished. Recording the proof consumption after the minting function execution is not working, since then one proof can be used multiple times.

One of the possible option is to add a rollback procedure for the proof recordings. In this case the proof should be forwarded to mint and in case it fails, we need to call a new rollback_proof method with the proof.

CC @mfornet @abacabadabacaba

mfornet · 2021-03-09T09:41:10Z

@djsatok my understanding from how the runtime works, is that if a promise fails, the full transaction is reverted, so the call to record_proof is also reverted. If there is not enough funds attached for the mint function, then it will fail and the whole deposit transaction is reverted.

Bridge factory checks that there's a user account, so no attached_deposit required

Bridge factory doesn't check this, this is checked on the BridgeToken once the mint function is called.

mfornet · 2021-03-09T10:09:01Z

After reading this SO answer I see the whole transaction is not reverted.

One possible solution is to record the proof on the BridgeToken instead of storing it on the factory. We pass the proof to mint and verify storage deposit is enough to have the account and to store the proof. In this case the function storage_balance_bounds from StorageManagement on BridgeToken will have unbounded upperbound.

@abacabadabacaba @djsatok

alexauroradev · 2021-03-10T01:15:24Z

Discussed the above mentioned issue with reverting the deposit transaction. Agreed solution is to require the attachmend of enough $NEAR to cover the storage of the proof and the creation of a new account (even if the account already exists). The appropriate assert should be added to the deposit method.

Perhaps in future we should implement the reverting mechanism, though it would still have some places to shoot in a leg, like attaching enough gas to pay for the reverting receipt.

Compile contracts

abacabadabacaba · 2021-03-10T15:18:47Z

Shouldn't we call mint with fixed amount of gas? Otherwise, a malicious user may attach too little gas to make it revert while the proof is still recorded.

mfornet · 2021-03-10T15:22:43Z

Shouldn't we call mint with fixed amount of gas? Otherwise, a malicious user may attach too little gas to make it revert while the proof is still recorded.

~~Done here~~

Notice the restriction is even higher. We enforce the attached deposit should be enough to register the account on the BridgeToken (even if it is already registered). Deposit that is not used is refunded to the signer on the BridgeToken contract.

Disregard previous comment, I read attached deposit instead of attached gas.

mfornet · 2021-03-10T15:34:19Z

Shouldn't we call mint with fixed amount of gas? Otherwise, a malicious user may attach too little gas to make it revert while the proof is still recorded.

Either we enforce a minimum amount of gas (which should be enough for the worst case)
or we assert that only the recipient of the transaction can call the method.

The drawback of the second approach, is that right now we currently don't require the recipient to even exist, and that won't be the case anymore.

abacabadabacaba · 2021-03-10T15:41:44Z

Why not just call mint with fixed amount of gas which should be enough? If there is not enough gas in finish_deposit for this, it will fail and the proof will not be recorded.

abacabadabacaba · 2021-03-10T16:04:16Z

Also, I see that this PR removes lock functions. Why doesn't it also remove unlock functions? These functions are useless without each other, aren't they?

abacabadabacaba · 2021-03-10T18:06:08Z

bridge-token-factory/build.sh

@@ -11,7 +11,7 @@ docker run \
     -w /host/bridge-token-factory \
     -e RUSTFLAGS='-C link-arg=-s' \
     nearprotocol/contract-builder \
-     cargo +stable build --target wasm32-unknown-unknown --release
+     /bin/bash -c "rustup target add wasm32-unknown-unknown; cargo build --target wasm32-unknown-unknown --release"


Why is this needed?

wasm32-unknown-unknown is not installed in the current docker image for the latest toolchain. Notice it haven't been updated for 8 months. The Dockerfile is correct but we need to publish the new image. Filed issue at near/near-sdk-rs#310

The error I'm getting right now:

Compiling bridge-token-factory v0.1.0 (/host/bridge-token-factory) error[E0463]: can't find crate for `std` | = note: the `wasm32-unknown-unknown` target may not be installed error: aborting due to previous error For more information about this error, try `rustc --explain E0463`. error: could not compile `bridge-token-factory`.

abacabadabacaba · 2021-03-10T18:07:54Z

bridge-token/src/lib.rs


 const NO_DEPOSIT: Balance = 0;

 #[near_bindgen]
-#[derive(BorshDeserialize, BorshSerialize)]
+#[derive(BorshDeserialize, BorshSerialize, PanicOnDefault)]


Please add PanicOnDefault to BridgeTokenFactory as well.

Done: 48eeea8

abacabadabacaba · 2021-03-10T18:09:39Z

bridge-token/src/lib.rs

+        decimals: Option<u8>,
+    ) {
+        // Only owner can change the metadata
+        assert_eq!(env::current_account_id(), env::signer_account_id());


Doesn't using signer_account_id have the same problems as using tx.origin in Solidity? Please change to predecessor_account_id.

Solved in 7482c5d

Moreover same issue existed with current implementation of AdminControlled which is fixed here: Near-One/rainbow-bridge#524. It is already fixed, and added PR to use respective version: dbc227b

abacabadabacaba · 2021-03-10T18:12:13Z

bridge-token/src/lib.rs

+            recipient,
+            &self.controller,
+            NO_DEPOSIT,
+            env::prepaid_gas() / 2,


This may run out of gas, in which case the tokens will be destroyed.

Done d0b92af

Together with fixed gas to schedule finish_withdraw / finish_deposit / mint

sept-en · 2021-03-10T22:03:06Z

bridge-token-factory/tests/token_transfer.rs

+        )
+    };
+}
+
 #[test]
 fn test_eth_token_transfer() {
    let (user, factory) = setup_token_factory();
    let root = "root".to_string();


This variable is redundant and is not used.

Solved in 3bd8556

This prevent loosing funds, by calling finish_withdraw.

abacabadabacaba

LGTM

Switching to new version of near-sdk-rs and new FT standard in Bridge…

698b768

…Token

alexauroradev reviewed Mar 4, 2021

View reviewed changes

bridge-token/src/lib.rs Outdated Show resolved Hide resolved

bridge-token/src/lib.rs Outdated Show resolved Hide resolved

ilblackdragon added 2 commits March 4, 2021 10:23

Fix up bridge token factory for NEP-141

4808613

First stub at updating sim tests

cc010f1

alexauroradev self-requested a review March 4, 2021 22:12

alexauroradev reviewed Mar 4, 2021

View reviewed changes

bridge-token-factory/src/lib.rs Outdated Show resolved Hide resolved

bridge-token-factory/src/lib.rs Outdated Show resolved Hide resolved

bridge-token/src/lib.rs Outdated Show resolved Hide resolved

sept-en mentioned this pull request Mar 5, 2021

ERC20Locker: make AdminControlled. Add tests. #36

Merged

mfornet added 2 commits March 5, 2021 10:17

Refund attached_deposito on mint and withdraw

fea2b94

Compile factory contract

b6b7422

Leverage rust enum types for better interface of ResultType Formatting

Address comments

cd20b41

mfornet reviewed Mar 5, 2021

View reviewed changes

mfornet added 3 commits March 8, 2021 21:06

use appropriate toolchain

2d6c358

Address comments

06b5737

Merge branch 'master' into ft-141

b7b3c78

mfornet added 2 commits March 10, 2021 15:58

Fix test

7213dff

Merge branch 'master' into ft-141

09649a7

Compile contracts

mfornet force-pushed the ft-141 branch from a27ca86 to 09649a7 Compare March 10, 2021 15:11

alexauroradev requested a review from abacabadabacaba March 10, 2021 16:50

abacabadabacaba reviewed Mar 10, 2021

View reviewed changes

sept-en reviewed Mar 10, 2021

View reviewed changes

mfornet added 8 commits March 11, 2021 16:35

Use PanicOnDefault

48eeea8

Remove functions related to lock/unlock

bafb25c

Dont allow pausing withdraw on the Factory

24e4b8a

This prevent loosing funds, by calling finish_withdraw.

Don't use signer_account_id

7482c5d

Fix tests

3bd8556

Use fixed version of AdminControlled

dbc227b

Compile contracts and fix tests.

48abd4b

Use fixed gas to schedule transaction in other contracts

d0b92af

mfornet mentioned this pull request Mar 11, 2021

Fix pinned dependencies Near-One/rainbow-bridge#528

Open

Use updated eth-types from rainbow-bridge

6855221

abacabadabacaba approved these changes Mar 12, 2021

View reviewed changes

mfornet merged commit d602235 into master Mar 12, 2021

This was referenced Mar 12, 2021

Move from NEP-21 standard of the token to NEP-141 #21

Closed

Update the storage requirements #22

Closed

mfornet deleted the ft-141 branch April 20, 2021 18:11

Switch to FT-141 for BridgeToken #39

Switch to FT-141 for BridgeToken #39

Conversation

ilblackdragon commented Mar 4, 2021 • edited Loading

ilblackdragon commented Mar 4, 2021

alexauroradev commented Mar 4, 2021

sept-en commented Mar 5, 2021

mfornet commented Mar 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilblackdragon commented Mar 5, 2021

ilblackdragon commented Mar 5, 2021 via email

ilblackdragon commented Mar 5, 2021 via email

mfornet commented Mar 5, 2021 • edited Loading

ilblackdragon commented Mar 8, 2021 via email

alexauroradev commented Mar 8, 2021 • edited Loading

alexauroradev commented Mar 8, 2021

alexauroradev commented Mar 8, 2021 • edited Loading

alexauroradev commented Mar 8, 2021

alexauroradev commented Mar 9, 2021

mfornet commented Mar 9, 2021

mfornet commented Mar 9, 2021

alexauroradev commented Mar 10, 2021

abacabadabacaba commented Mar 10, 2021

mfornet commented Mar 10, 2021 • edited Loading

mfornet commented Mar 10, 2021

abacabadabacaba commented Mar 10, 2021

abacabadabacaba commented Mar 10, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abacabadabacaba left a comment

Choose a reason for hiding this comment

ilblackdragon commented Mar 4, 2021 •

edited

Loading

mfornet commented Mar 5, 2021 •

edited

Loading

alexauroradev commented Mar 8, 2021 •

edited

Loading

alexauroradev commented Mar 8, 2021 •

edited

Loading

mfornet commented Mar 10, 2021 •

edited

Loading