-
Notifications
You must be signed in to change notification settings - Fork 244
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
Conversation
🦋 Changeset detectedLatest commit: 6121187 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
Not sure why the |
cc: @frol |
There was a problem hiding this 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?
The reason we haven't created its own package is that the functionality is tightly coupled with the |
@frol The CI is failing because of other changes that have been recently merged into |
@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 |
I feel that P.S. It is not a requirement for this PR to be merged, though. |
CI errors are not a glitch, there is something wrong going on. May I ask you guys to look more closely into it? 🙏 |
Hey @frol, you can find the same error in #1189 that's been merged a few days ago:
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. |
@arrusev @vikinatora The issue has been solved in main branch, please, rebase and let's merge this. |
Signed-off-by: Anton Rusev <[email protected]>
Signed-off-by: Anton Rusev <[email protected]>
Signed-off-by: Anton Rusev <[email protected]>
Signed-off-by: Anton Rusev <[email protected]>
Signed-off-by: Anton Rusev <[email protected]>
Signed-off-by: Anton Rusev <[email protected]>
Signed-off-by: Anton Rusev <[email protected]>
c7166bc
to
66309f7
Compare
@frol build is passing, I believe we can merge this |
Pre-flight checklist
pnpm changeset
to create achangeset
JSON document appropriate for this change.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:
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 statesfastnear/fast-near#13