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

fix: read/write context/permission #93

Merged
merged 20 commits into from
Mar 27, 2023

Conversation

da1suk8
Copy link
Member

@da1suk8 da1suk8 commented Feb 27, 2023

Description

This PR is related to Finschia/cosmwasm#263, Finschia/cosmwasm#270

The read/write permissions of dynamic link caller functions were not properly inherited by callee environment, so fixed this.

There are a total of four possible combinations of caller and callee function read-write permissions.
The read-write permission is inherited by callee with the following rules.

If the caller's permission is read-only and the function of callee is read-write, an error occurs.
If the caller's permission is read-only and the function of callee is read-only, the callee's permission is set to read-only.
If the caller's permission is read-write and the function of callee is read-write, the callee's permission is set to read-write.
If the caller's permission is read-write and the function of callee is read-only, the callee's permission is set to read-only.

When you build a contract that has the role of callee for dynamic link in cosmwasm, the following function is exported in WASM.

  (export "_get_callable_points_properties" (func ..))

The return value of the function is a Map that takes the function name as a key and returns a read-write-permission.
True is read-only.
(This example is number contract)

{
    "add": CalleeProperty { is_read_only: false }, 
    "number": CalleeProperty { is_read_only: true }, 
    "sub": CalleeProperty { is_read_only: false }, 
    "mul": CalleeProperty { is_read_only: false }, 
}

The read-write permission is brought from this function and inherited by the instance of callee.

Types of changes

  • Bug fix (changes which fixes an issue)
  • New feature (changes which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • ETC (build, ci, docs, perf, refactor, style, test)

Checklist

The read/write permissions of dynamic link caller functions
were not properly inherited by callee environment, so fixed this
@da1suk8 da1suk8 marked this pull request as ready for review February 28, 2023 02:18
@da1suk8 da1suk8 self-assigned this Feb 28, 2023
Copy link
Contributor

@loloicci loloicci left a comment

Choose a reason for hiding this comment

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

Write a test if it is able.
And, isn't it needed some verification in storing the code? If it has callable point export, the listing function export is also needed after it.

libwasmvm/src/api.rs Outdated Show resolved Hide resolved
libwasmvm/src/api.rs Outdated Show resolved Hide resolved
libwasmvm/src/api.rs Outdated Show resolved Hide resolved
libwasmvm/src/api.rs Outdated Show resolved Hide resolved
libwasmvm/src/api.rs Outdated Show resolved Hide resolved
rename fuction name

add error handling instead of unwrap()
@da1suk8 da1suk8 requested a review from loloicci March 1, 2023 01:58
@da1suk8
Copy link
Member Author

da1suk8 commented Mar 1, 2023

@loloicci

I am thinking of writing the test in lbm-sdk.
Also, verification in storing the code is considered to be done on the cosmwasm side.

@loloicci loloicci added the dynamic_link relate the dynamic link call feature label Mar 2, 2023
libwasmvm/src/api.rs Outdated Show resolved Hide resolved
Because it is possible to define read-write functions
in the _list_callable_points function and change the storage
(e.g. if you write WASM directly etc)
libwasmvm/src/api.rs Outdated Show resolved Hide resolved
libwasmvm/src/api.rs Outdated Show resolved Hide resolved
Co-authored-by: TAKASE Ryo <loloicci@loloicci.dev>
libwasmvm/src/api.rs Outdated Show resolved Hide resolved
libwasmvm/src/api.rs Outdated Show resolved Hide resolved
raise error when inheriting from read-only to read-write permission
@da1suk8 da1suk8 requested review from loloicci and Kynea0b March 9, 2023 01:41
libwasmvm/src/api.rs Outdated Show resolved Hide resolved
prepare a separate get_read_write_permission function
@da1suk8 da1suk8 requested a review from Kynea0b March 9, 2023 08:31
libwasmvm/src/api.rs Outdated Show resolved Hide resolved
@da1suk8 da1suk8 requested a review from Kynea0b March 10, 2023 01:54
@Kynea0b
Copy link

Kynea0b commented Mar 14, 2023

@da1suk8
There are only 4 patterns of authority setting rules, so I want you to write them explicitly in the description.
For example:
If the caller's permission is readOnly and the callee's is readwrite, the calllee's is set to readOnly.

libwasmvm/src/api.rs Outdated Show resolved Hide resolved
Copy link

@Kynea0b Kynea0b left a comment

Choose a reason for hiding this comment

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

LGTM

@da1suk8
Copy link
Member Author

da1suk8 commented Mar 16, 2023

CI is failing for the same reason as below.

#89 (comment)

CI fails with a race error and this error is caused even without this PR. A test added with this PR catch the race error of the cGetContractEnv. And we will solve this error in another PR.

api/lib_test.go Outdated Show resolved Hide resolved
@loloicci
Copy link
Contributor

When you build a contract that has the role of callee for dynamic link in cosmwasm, the following function is exported in WASM.

(export "_list_callable_points" (func ..))
The return value of the function is a Map that takes the function name as a key and returns a read-write-permission.
True is read-only.
(This example is number contract)

{"number": true, "add": false, "mul": false, "sub": false}

_list_callable_point does not describe which (readonly or read-write) is true and which is false. How about changing _list_callable_point_is_readonly?

@da1suk8
Copy link
Member Author

da1suk8 commented Mar 24, 2023

Finschia/cosmwasm#270 (comment)

Along with the modification of cosmwasm, I also modified here.

@da1suk8
Copy link
Member Author

da1suk8 commented Mar 27, 2023

Added 1a56bcd because the name of the variable was not appropriate.

@loloicci
Copy link
Contributor

LGTM

@da1suk8 da1suk8 merged commit 614db1c into Finschia:dynamic_link Mar 27, 2023
@da1suk8 da1suk8 deleted the fix/read-write-context branch March 27, 2023 04:28
da1suk8 added a commit to da1suk8/wasmd that referenced this pull request Mar 31, 2023
da1suk8 added a commit to da1suk8/lbm-sdk that referenced this pull request Apr 7, 2023
da1suk8 added a commit to Finschia/wasmd that referenced this pull request Apr 12, 2023
* feat: apply the wasm module of Finschia/finschia-sdk#936

* ci: add benchmarking job

* fix: update error Message via Finschia/wasmvm#93

* ci test

* fix: go fmt

* build: update commit-hash of wasmvm

* docs: update CHANGELOG

* test: update on expected gas
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dynamic_link relate the dynamic link call feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants