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

feat: implement local execution of view methods #1172

Merged
merged 8 commits into from
Nov 15, 2023

Conversation

arrusev
Copy link
Contributor

@arrusev arrusev commented Aug 4, 2023

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • Commit messages follow the conventional commits spec
  • If this is a code change: I have written unit tests.
  • If this changes code in a published package: I have run pnpm changeset to create a changeset JSON document appropriate for this change.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Implementation of the idea https://near.social/devgovgigs.near/widget/gigs-board.pages.Post?id=455

This PR modifies the accounts package in a way that reduces the unnecessary RPC calls to contract view methods for the same blockId(number, hash). It is useful when you make more than one contract call for a specific blockId.

It is an opt-in feature that could be enabled when instantiating a contract. Disabled by default.

The first time a contract call is made, several things are stored locally in a LRU map(Least Recently Used) for the specified blockId:

  • contract code
  • contract state
  • block number
  • block timestamp

Each subsequent request for this blockId will load the above data in a WASM environment and execute the contract view method locally, without making any RPC calls

It has a fallback mechanism where if the local view execution fails, it will do a normal RPC contract call.

Test Plan

Added unit/e2e tests

Related issues/PRs

In the current implementation there is a limitation for contract state < 50kb. There is an ongoing discussion with the fast-near team in order to support bigger contract states

fastnear/fast-near#13

@changeset-bot
Copy link

changeset-bot bot commented Aug 4, 2023

🦋 Changeset detected

Latest commit: 6121187

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@near-js/accounts Patch
@near-js/cookbook Patch
near-api-js Patch
@near-js/wallet-account Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@arrusev
Copy link
Contributor Author

arrusev commented Aug 7, 2023

Not sure why the provider: json rpc query view_state test is failing. It is not affected by our changes

@vikinatora
Copy link
Collaborator

cc: @frol

frol
frol previously approved these changes Sep 11, 2023
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

Sorry for the delay!!!

This looks fantastic! Please, rebase to the master branch (GitHub went crazy and says that your branch is not synced with the base branch), and I am ready to merge it once CI is green (hopefully it will be fine after the rebase).

P.S. It would be great to extract local view execution into its own package. What do you think?

@arrusev
Copy link
Contributor Author

arrusev commented Sep 12, 2023

P.S. It would be great to extract local view execution into its own package. What do you think?

The reason we haven't created its own package is that the functionality is tightly coupled with the accounts package and it cannot be used on its own. If you think it would be better to have its own package, we can extract it. What do you think?

@vikinatora
Copy link
Collaborator

@frol The CI is failing because of other changes that have been recently merged into master. You can see that the CI fails with the same error in other PRs.

@frol
Copy link
Collaborator

frol commented Sep 12, 2023

You can see that the CI fails with the same error in other PRs.

@vikinatora I don't see other PRs failing with the same errors. The errors in other PRs are totally different. I feel it might be a network glitch, so I re-run the CI, but if it fails, please, take a closer look

@frol
Copy link
Collaborator

frol commented Sep 12, 2023

The reason we haven't created its own package is that the functionality is tightly coupled with the accounts package and it cannot be used on its own. If you think it would be better to have its own package, we can extract it. What do you think?

I feel that runtime and storage are quite independent. I can easily see fast-near and near-api-js sharing this module instead of copy-pasting. There could be potentially some other project that would also need this runtime.

P.S. It is not a requirement for this PR to be merged, though.

@frol
Copy link
Collaborator

frol commented Sep 12, 2023

CI errors are not a glitch, there is something wrong going on. May I ask you guys to look more closely into it? 🙏

@vikinatora
Copy link
Collaborator

vikinatora commented Sep 13, 2023

Hey @frol, you can find the same error in #1189 that's been merged a few days ago:

@near-js/accounts:test:   ● json rpc query view_state
@near-js/accounts:test: 
@near-js/accounts:test:     expect(received).toEqual(expected) // deep equality
@near-js/accounts:test: 
@near-js/accounts:test:     - Expected  - 2
@near-js/accounts:test:     + Received  + 0
@near-js/accounts:test: 
@near-js/accounts:test:       Object {
@near-js/accounts:test:         "block_hash": Any<String>,
@near-js/accounts:test:         "block_height": Any<Number>,
@near-js/accounts:test:     -   "proof": Array [],
@near-js/accounts:test:         "values": Array [
@near-js/accounts:test:           Object {
@near-js/accounts:test:             "key": "bmFtZQ==",
@near-js/accounts:test:     -       "proof": Array [],
@near-js/accounts:test:             "value": "aGVsbG8=",
@near-js/accounts:test:           },
@near-js/accounts:test:         ],
@near-js/accounts:test:       }

I also built and ran the tests locally against both the current upstream master and @arrusev's LVE branch and they both fail with this error. We still believe that this test failure has nothing to do with our improvements.

@frol
Copy link
Collaborator

frol commented Oct 31, 2023

@arrusev @vikinatora The issue has been solved in main branch, please, rebase and let's merge this.

@vikinatora
Copy link
Collaborator

@frol build is passing, I believe we can merge this

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.

3 participants