Skip to content

Eth connector statistic#104

Merged
mrLSD merged 9 commits into
eth-connector-improves-after-review-20201-05-18from
eth-connector-statistic
May 24, 2021
Merged

Eth connector statistic#104
mrLSD merged 9 commits into
eth-connector-improves-after-review-20201-05-18from
eth-connector-statistic

Conversation

@mrLSD
Copy link
Copy Markdown
Member

@mrLSD mrLSD commented May 20, 2021

Calculate total unique accounts.

Added method:

  • get_accounts_counter

Covered with test

@mrLSD mrLSD requested a review from artob as a code owner May 20, 2021 19:52
@mrLSD mrLSD self-assigned this May 20, 2021
@mrLSD mrLSD added the C-enhancement Category: New feature or request label May 20, 2021
@mrLSD mrLSD requested review from birchmd, joshuajbouw and sept-en May 20, 2021 19:53
mrLSD added 2 commits May 20, 2021 22:54
…hub.com:aurora-is-near/aurora-engine into eth-connector-statistic
@mrLSD mrLSD mentioned this pull request May 20, 2021
Comment thread src/fungible_token.rs Outdated
Comment thread src/fungible_token.rs
Comment thread tests/test_connector.rs Outdated
.unwrap()
.parse()
.unwrap();
assert_eq!(counter, 2);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is the value 2? I only see one call to deposit so I would only expect there to be one account.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Please look to that test:

#[test]
fn test_near_deposit_balance_total_supply() {
    let (master_account, contract) = init(CUSTODIAN_ADDRESS);
    call_deposit_near(&contract, CONTRACT_ACC);

    let balance = get_near_balance(&master_account, DEPOSITED_RECIPIENT, CONTRACT_ACC);
    assert_eq!(balance, DEPOSITED_AMOUNT - DEPOSITED_FEE);

    let balance = get_near_balance(&master_account, CONTRACT_ACC, CONTRACT_ACC);
    assert_eq!(balance, DEPOSITED_FEE);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right, there is a fee as well, so one deposit creates two accounts. Thanks for clarifying; it might be worth saying this in a comment as well.

Also, I recommend extending the test to have a transfer between those two accounts after the deposit, and confirm that the count does not change (because the number of unique accounts is the same).

Copy link
Copy Markdown
Contributor

@sept-en sept-en left a comment

Choose a reason for hiding this comment

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

LGTM!
Still, let's put some attention to storage keys, otherwise, this could be quite confusing and hard to support later.

Comment thread src/storage.rs Outdated
Comment thread src/fungible_token.rs
mrLSD added 2 commits May 21, 2021 13:05
@mrLSD mrLSD requested review from birchmd and sept-en May 21, 2021 10:06
Copy link
Copy Markdown
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.

Generally looks good to me. Please see my note about extending the test.

Comment thread src/connector.rs Outdated
Comment thread tests/test_connector.rs Outdated
.unwrap()
.parse()
.unwrap();
assert_eq!(counter, 2);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right, there is a fee as well, so one deposit creates two accounts. Thanks for clarifying; it might be worth saying this in a comment as well.

Also, I recommend extending the test to have a transfer between those two accounts after the deposit, and confirm that the count does not change (because the number of unique accounts is the same).

Imptove comments

Co-authored-by: Michael Birch <michael@near.org>
Comment thread src/connector.rs Outdated
- get_accounts_counter - return le_bytes
Added:
- test_get_accounts_counter_and_transfer - check recalculation accounts
@mrLSD mrLSD requested a review from joshuajbouw May 24, 2021 10:01
@mrLSD mrLSD merged commit cf33465 into eth-connector-improves-after-review-20201-05-18 May 24, 2021
@mrLSD mrLSD deleted the eth-connector-statistic branch May 24, 2021 11:45
mrLSD added a commit that referenced this pull request May 26, 2021
* Adde improvements afterreview - account checking, msg len check, json lib

* Added json parser. Changed balance_of

* Fixed balance_of test

* json for ft_transfer & fix test

* Json args for ft_transfer_call & fixed tests

* Added json for FT storage methods

* Fix ft_resolve transfer promise result check. Removed total_supply_near

* Fix to NEP-145: storage_balance_of

* Fix storage_balance_of to NEP-145

* Eth connector statistic (#104)

* Added get_accounts_counter & tests

* Clippy fix

* Fixed: check accounts_contains_key & added comments

* Lint: cargo fmt

* Chanched key for Statistics

* Update src/connector.rs

Imptove comments

Co-authored-by: Michael Birch <michael@near.org>

* Changed:
- get_accounts_counter - return le_bytes
Added:
- test_get_accounts_counter_and_transfer - check recalculation accounts

Co-authored-by: Michael Birch <michael@near.org>

Co-authored-by: Michael Birch <michael@near.org>
mrLSD added a commit that referenced this pull request May 26, 2021
* Eugene's review 2021-05-08

* Eth connector improvements after review 2021-05-18 (#98)

* Adde improvements afterreview - account checking, msg len check, json lib

* Added json parser. Changed balance_of

* Fixed balance_of test

* json for ft_transfer & fix test

* Json args for ft_transfer_call & fixed tests

* Added json for FT storage methods

* Fix ft_resolve transfer promise result check. Removed total_supply_near

* Fix to NEP-145: storage_balance_of

* Fix storage_balance_of to NEP-145

* Eth connector statistic (#104)

* Added get_accounts_counter & tests

* Clippy fix

* Fixed: check accounts_contains_key & added comments

* Lint: cargo fmt

* Chanched key for Statistics

* Update src/connector.rs

Imptove comments

Co-authored-by: Michael Birch <michael@near.org>

* Changed:
- get_accounts_counter - return le_bytes
Added:
- test_get_accounts_counter_and_transfer - check recalculation accounts

Co-authored-by: Michael Birch <michael@near.org>

Co-authored-by: Michael Birch <michael@near.org>

Co-authored-by: Evgeny Ukhanov <mrlsd@ya.ru>
Co-authored-by: Michael Birch <michael@near.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-enhancement Category: New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants