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

Refactor with NEAR standard NEP141 #2

Merged
merged 48 commits into from
Jan 5, 2023

Conversation

mrLSD
Copy link
Collaborator

@mrLSD mrLSD commented Nov 28, 2022

Overview

To significantly simplify the contract, as well as to avoid potential errors and pitfalls associated with the implementation of the NEP-141, the contract is being refactored with a library near-standard dependency.

What is removed

All implementations related to Fungible TOkens. And introduced dependency from near-standard.

Breaking changes

NEP-141 basic (ft_transfer, ft_transfer_call) methods are now public, without access right control.
All related methods were also removed.

All types, that were related to NEP-141 implementation are deleted.

Tests

Tests refactored qith new types dependencies and with keep in mind of new changes

Gas cost

Gas cost increase x2 (twice) for migrations. It relates to a new organization of storage key.

@mrLSD mrLSD self-assigned this Nov 28, 2022
@mrLSD mrLSD marked this pull request as ready for review December 1, 2022 16:13
@mrLSD mrLSD requested review from joshuajbouw and birchmd December 1, 2022 16:23
Copy link
Contributor

@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.

Looks pretty good to me. A net decrease of almost 1500 lines of code should be good for us in the long run.

eth-connector-tests/src/migration.rs Show resolved Hide resolved
eth-connector-tests/src/migration.rs Show resolved Hide resolved
eth-connector/src/connector_impl.rs Show resolved Hide resolved
eth-connector-tests/src/migration.rs Show resolved Hide resolved
@mrLSD
Copy link
Collaborator Author

mrLSD commented Dec 5, 2022

@birchmd unfortunately, gas costs for migration increased twice (!). AFAIK it depends on how currently storage key is organized. Specifically proof keys.

@birchmd
Copy link
Contributor

birchmd commented Dec 7, 2022

A factor of 2 is a large increase. It's not clear to me why this increase happens. You say it's because of the proof keys, but it looks like both before and after this change they were contained in a LookupMap<String, bool>. Could you elaborate more on what difference causes the gas increase?

Also, is this factor of 2 increase a problem for this migration?

@joshuajbouw
Copy link

@birchmd @mrLSD gas cost possibly may have increased with Workspace 0.7.0?

Copy link

@joshuajbouw joshuajbouw left a comment

Choose a reason for hiding this comment

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

That was a larger PR than I expected. The tests are still failing, that really needs to get fixed. Large gas costs may have measured higher in workspaces perhaps? Or, NEAR's contract impl have a bit more checks in it. I don't really see what could've caused it but generally, if there are gas cost increases it is quite important to find out why.

@mrLSD
Copy link
Collaborator Author

mrLSD commented Dec 12, 2022

@birchmd @mrLSD gas cost possibly may have increased with Workspace 0.7.0?

It was changed after the proposal in: near/near-workspaces-rs#255 ,
near/near-workspaces-rs#253

Opened 2 issues about that. Currently no success.

@joshuajbouw
Copy link

Whats the status on this?

@mrLSD mrLSD merged commit c6f7359 into master Jan 5, 2023
@mrLSD mrLSD deleted the feat/use-near-standard-nep141-implementation branch January 5, 2023 23:48
@mrLSD mrLSD added this to the v0.2.0 milestone Mar 17, 2023
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.

4 participants