near and storage tracing - wip #7

robert-zaremba · 2021-03-29T13:59:09Z

No description provided.

ref-exchange/src/account_deposit.rs

robert-zaremba · 2021-03-29T14:01:16Z

ref-exchange/src/account_deposit.rs

-    pub amount: Balance,
+    /// NEAR sent to the exchange.
+    /// Used for storage and trading.
+    pub ynear: Balance,


renaming amount -> ynear to enhance code readability.

we will use near_amount

robert-zaremba · 2021-03-29T14:04:42Z

ref-exchange/src/lib.rs

@@ -193,6 +188,8 @@ impl Contract {
        deposits.add(token_out.as_ref().clone(), amount_out);
        self.deposited_amounts.insert(&sender_id, &deposits);
        self.pools.replace(pool_id, &pool);
+        deposits.update_storage(start_storage);
+        self.deposited_amounts.insert(&sender_id, &deposits);


To verify: I think we need to insert 2 times:

once in line 194 (prev)

second time here.
This is because we won't trace properly new storage until we insert the record into the storage tree.
Alternative would be to refactor the AccountDeposit and move ynear to another, top-level map.

robert-zaremba · 2021-03-29T14:06:21Z

ref-exchange/src/account_deposit.rs

+    pub(crate) fn update_storage(&self, from: &AccountDeposit, tx_start_storage: StorageUsage) {
+        let mut d = self.get_account_depoists(from);
+        d.update_storage(tx_start_storage);
+        self.deposited_amounts.insert(from, &d);


how about renaming self.deposited_amounts to self.deposits?

rename to accounts

robert-zaremba · 2021-03-29T14:06:53Z

ref-exchange/src/lib.rs

@@ -109,7 +109,7 @@ impl Contract {
        let mut pool = self.pools.get(pool_id).expect("ERR_NO_POOL");
        // Add amounts given to liquidity first. It will return the balanced amounts.
        pool.add_liquidity(&sender_id, &mut amounts);
-        let mut deposits = self.deposited_amounts.get(&sender_id).unwrap_or_default();
+        let mut deposits = self.get_account_depoists(&sender_id);


add liquidity should early fail if account is not registered.

robert-zaremba · 2021-03-29T14:11:35Z

ref-exchange/src/lib.rs

-            storage_cost <= env::attached_deposit(),
-            "ERR_STORAGE_DEPOSIT"
-        );
+        // TODO: update deposit handling
        let refund = env::attached_deposit() - storage_cost;
        if refund > 0 {
            Promise::new(env::predecessor_account_id()).transfer(refund);


refund won't work, because I added assert_one_yocto().

Moreover - it's easier to standarize here to use internal storage tracking + assert_one_yocto, rather than asking user for unknown required NEAR attachment.

ilblackdragon · 2021-04-02T04:42:57Z

ref-exchange/src/account_deposit.rs

-const MIN_ACCOUNT_DEPOSIT_LENGTH: u128 = MAX_ACCOUNT_LENGTH + 16 + 4;
+/// Minimum Account Storage used for account registration.
+/// 64 (AccountID bytes) + 2*8 (int32) + byte
+pub const INIT_ACCOUNT_STORAGE: StorageUsage = 64 + 16 + 4;


Let's bring constants like this - https://github.com/ref-finance/ref-contracts/pull/6/files#diff-445ab9fc382f558a0e9d7831b135b640b79e74be187c7cbdd435b7ecf6e108bbR13 to explain what is happening

ref-exchange/src/account_deposit.rs

ilblackdragon

Hm, I'm not 100% sure you actually handled the upgrading - i see direct self.account.get calls.
I think the easiest way is to keep account_deposit as old field and create new lookup map with VersionedAccount. And just when loading - check back if it exists and upgrade.

And let's have a test_upgrade to actually test this upgrade with an old contract that has some state.

ref-exchange/src/account_deposit.rs

ilblackdragon · 2021-04-20T17:26:23Z

ref-exchange/src/account_deposit.rs

+/// Account deposits information and storage cost.
+#[derive(BorshSerialize, BorshDeserialize, Default)]
+#[cfg_attr(feature = "test", derive(Clone))]
+pub struct AccountDepositV2 {


let's rename to Account?

The v1 structure is already named AccountDeposit. So - do you suggest to rename both?

ilblackdragon · 2021-04-20T17:27:52Z

ref-exchange/src/account_deposit.rs

+                    return a;
+                }
+                // migrate from V1
+                a.storage_used = U64_STORAGE;


hm, this is (MIN_ACCOUNT_DEPOSIT_LENGTH + self.tokens.len() as u128 * (MAX_ACCOUNT_LENGTH + 16)) * env::storage_byte_cost() (old storage_usage)

BTW - we don't multiply by env::storage_byte_cost() here.

Ah, I remember why I don't include the AccountDeposit storage here, please see the comment below (#7 (comment))

ilblackdragon · 2021-04-20T17:28:26Z

ref-exchange/src/account_deposit.rs

-            * env::storage_byte_cost()
+        let s = self.storage_used
+            + INIT_ACCOUNT_STORAGE  // empty account storage
+            + (ACC_ID_STORAGE + U64_STORAGE) * self.tokens.len() as u64; // self.tokens storage


why not just count this inside storage_used? so storage_usage * cost is just self.storage_used?
also it seems like you are missing cost of storage in general here.

Because, in self.storage_used we only account storage used outside of this structure. This is to avoid "double" storing the account. See here: #7 (comment)

ilblackdragon · 2021-04-20T17:30:30Z

ref-exchange/src/account_deposit.rs

+    #[payable]
+    pub fn deposit_near(&mut self) {
+        let sender_id = env::predecessor_account_id();
+        let mut acc = self.get_account(&sender_id);


should we register here if not registered? it seems like this will be easier to use than storage_deposit in general case.

I was thinking about it. We have a standard for user / account registration. This function is to make it more clear to deposit more NEAR and not confuse with registration. However if you think that by allowing a registration here, we will improve UX then we can do that.

ilblackdragon · 2021-04-20T17:31:40Z

ref-exchange/src/account_deposit.rs

+// pub const NEAR: AccountId = "".to_string();
+
+#[derive(BorshDeserialize, BorshSerialize)]
+pub enum AccountDeposit {


I like more VersionedAccount and Account for the current version.
This way everywhere in the code except in the places where it's used - it's just Account. And VersionedAccount is where need to store/load/upgrade

So, you want to have a LegacyAccount and Account? What if we will do another migration? I think having a consecutive versions in struct name is more clear. We will mark all old version Legacy in the comments.

robert-zaremba · 2021-04-22T14:08:14Z

Hm, I'm not 100% sure you actually handled the upgrading - i see direct self.account.get calls.
I think the easiest way is to keep account_deposit as old field and create new lookup map with VersionedAccount. And just when loading - check back if it exists and upgrade.

I've asked @evgenykuzyakov about possible strategies few weeks ago. I think if we won't use indexer then then doing byte computation for figuring out the structure version is too error prone.

So, depending which strategy we will take. If we want to do it lazy (so user will do migration when he will use the account next time) then we will need to duplicate the accounts - if we will use indexer then we will do a migration function. We had a chat about it in telegram. Here I wanted to verify the AccountDeposit structure and update.

referencedev · 2021-05-17T05:05:33Z

#27 adds storage handling without needing to upgrade Account data structure. This fixes the issue short term and removes need to keep track storage_used from it for now.

Accumulative patches in April

robert-zaremba requested a review from ilblackdragon March 29, 2021 13:59

robert-zaremba commented Mar 29, 2021

View reviewed changes

ref-exchange/src/account_deposit.rs Outdated Show resolved Hide resolved

robert-zaremba commented Mar 29, 2021

View reviewed changes

robert-zaremba force-pushed the near-deposits branch from 68c7bc8 to 379ebde Compare March 29, 2021 14:09

robert-zaremba commented Mar 29, 2021

View reviewed changes

ilblackdragon mentioned this pull request Apr 2, 2021

Adding storage used field #6

Closed

ilblackdragon reviewed Apr 2, 2021

View reviewed changes

ref-exchange/src/account_deposit.rs Outdated Show resolved Hide resolved

robert-zaremba force-pushed the near-deposits branch from 379ebde to a6ba015 Compare April 9, 2021 15:05

robert-zaremba added 5 commits April 12, 2021 16:11

near and storage tracing

e23a40e

wip

303148d

Add NEAR deposits

d4b3247

update the storage_used function

2b51e5c

add migration

ec470c8

robert-zaremba force-pushed the near-deposits branch from 9eeb1b3 to ec470c8 Compare April 12, 2021 14:15

ilblackdragon reviewed Apr 20, 2021

View reviewed changes

review updates

7e8b9ae

robert-zaremba force-pushed the near-deposits branch from 6362de5 to 7e8b9ae Compare April 22, 2021 14:03

marco-sundsk added a commit that referenced this pull request May 5, 2022

Merge pull request #7 from marco-sundsk/accumulative-patches-apr

a20ad7f

Accumulative patches in April

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

near and storage tracing - wip #7

near and storage tracing - wip #7

robert-zaremba commented Mar 29, 2021

robert-zaremba Mar 29, 2021

robert-zaremba Mar 30, 2021

robert-zaremba Mar 29, 2021

robert-zaremba Mar 29, 2021

robert-zaremba Mar 30, 2021

robert-zaremba Mar 29, 2021

robert-zaremba Mar 29, 2021

robert-zaremba Mar 29, 2021

ilblackdragon Apr 2, 2021

ilblackdragon left a comment

ilblackdragon Apr 20, 2021

robert-zaremba Apr 22, 2021

ilblackdragon Apr 20, 2021

robert-zaremba Apr 22, 2021

robert-zaremba Apr 22, 2021

ilblackdragon Apr 20, 2021

robert-zaremba Apr 22, 2021

ilblackdragon Apr 20, 2021

robert-zaremba Apr 22, 2021

ilblackdragon Apr 20, 2021

robert-zaremba Apr 22, 2021

robert-zaremba commented Apr 22, 2021

referencedev commented May 17, 2021

near and storage tracing - wip #7

Are you sure you want to change the base?

near and storage tracing - wip #7

Conversation

robert-zaremba commented Mar 29, 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

ilblackdragon 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

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

robert-zaremba commented Apr 22, 2021

referencedev commented May 17, 2021