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.

Sign up for GitHub

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

Jump to bottom

feat(gstd, backend): Introduce gr_env_vars sys-call #3403

Merged
merged 20 commits into from
Oct 23, 2023
Merged

feat(gstd, backend): Introduce gr_env_vars sys-call #3403

merged 20 commits into from
Oct 23, 2023

Conversation

DennisInSky
Copy link
Member

@DennisInSky DennisInSky commented Oct 11, 2023 •
edited by shamilsan
Loading

Introduce gr_env_vars sys-call providing user space with various system settings. The settings are supposed to be extandable via versioning so old clients can still obtain the settings w/o any issues when backend introduces new versions

@DennisInSky DennisInSky self-assigned this Oct 11, 2023
@DennisInSky DennisInSky added the A0-pleasereview PR is ready to be reviewed by the team label Oct 11, 2023
@DennisInSky DennisInSky changed the title Introduce gr_exec_settings sys-call feat(gstd, backend): Introduce gr_exec_settings sys-call Oct 11, 2023
DennisInSky
DennisInSky commented Oct 11, 2023
View reviewed changes
core/src/exec_settings.rs Outdated Show resolved Hide resolved
pallets/gear/src/benchmarking/syscalls.rs Show resolved Hide resolved
ark0f
ark0f requested changes Oct 11, 2023
View reviewed changes
core-backend/src/mock.rs Outdated Show resolved Hide resolved
gsys/src/lib.rs Outdated Show resolved Hide resolved
utils/wasm-instrument/src/syscalls.rs Outdated Show resolved Hide resolved
@DennisInSky DennisInSky requested a review from ark0f October 11, 2023 22:02
@NikVolf
Copy link
Member

NikVolf commented Oct 16, 2023

@ark0f re-review?

ark0f
ark0f requested changes Oct 16, 2023
View reviewed changes
core-backend/src/error.rs Outdated Show resolved Hide resolved
gsys/src/lib.rs Outdated Show resolved Hide resolved
gcore/src/exec.rs Outdated Show resolved Hide resolved
gcore/src/exec.rs Outdated Show resolved Hide resolved
utils/wasm-instrument/src/syscalls.rs Outdated Show resolved Hide resolved
core/src/exec_settings.rs Outdated Show resolved Hide resolved
gcore/src/exec.rs Outdated Show resolved Hide resolved
@breathx breathx added A3-gotissues PR occurred to have issues after the review and removed A0-pleasereview PR is ready to be reviewed by the team labels Oct 16, 2023
@DennisInSky DennisInSky requested a review from ark0f October 17, 2023 12:18
@DennisInSky DennisInSky added the A0-pleasereview PR is ready to be reviewed by the team label Oct 17, 2023
ark0f
ark0f approved these changes Oct 18, 2023
View reviewed changes
gsys/src/lib.rs Outdated Show resolved Hide resolved
utils/wasm-instrument/src/syscalls.rs Outdated Show resolved Hide resolved
utils/wasm-instrument/src/syscalls.rs Show resolved Hide resolved
breathx
breathx requested changes Oct 18, 2023
View reviewed changes
Copy link
Member

@breathx breathx left a comment

Choose a reason for hiding this comment

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

Requesting changes due to gas_to_value converter
to avoid using enums you can hack the system by assuming multiplier as u128 where last 64bits are u64, first - just defining byte :/

And one more thing about naming: does "settings" fit well? Maybe "params"? like env_params... to be considered

Otherwise LveryGTM

pallets/gear/src/benchmarking/syscalls.rs Show resolved Hide resolved
core-backend/src/error.rs Outdated Show resolved Hide resolved
core-processor/src/configs.rs Outdated Show resolved Hide resolved
core-processor/src/ext.rs Outdated Show resolved Hide resolved
gsys/src/lib.rs Outdated Show resolved Hide resolved
gsys/src/lib.rs Outdated Show resolved Hide resolved
gsys/src/lib.rs Outdated Show resolved Hide resolved
pallets/gear/src/benchmarking/mod.rs Outdated Show resolved Hide resolved
utils/wasm-instrument/src/syscalls.rs Outdated Show resolved Hide resolved
@DennisInSky DennisInSky requested a review from breathx October 19, 2023 17:31
breathx
breathx approved these changes Oct 20, 2023
View reviewed changes
Copy link
Member

@breathx breathx left a comment

Choose a reason for hiding this comment

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

Awesome.

So how do you feel about renaming: gr_exec_settings into gr_env_parameters, or even maybe gr_env_vars ? Cause it most likely variable env params rather than settings of some standalone execution.

pallets/gear/src/benchmarking/mod.rs Outdated Show resolved Hide resolved
@breathx breathx added A2-mergeoncegreen PR is ready to merge after CI passes and removed A3-gotissues PR occurred to have issues after the review labels Oct 22, 2023
techraed
techraed reviewed Oct 23, 2023
View reviewed changes
Copy link
Member

@techraed techraed left a comment

Choose a reason for hiding this comment

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

I'm not sure that we need to generate gr_exec_settings from wasm-gen for loader and fuzzer. What about adjusting wasm-gen configs for those test tools accordingly?

utils/wasm-instrument/src/syscalls.rs Outdated Show resolved Hide resolved
@DennisInSky DennisInSky merged commit aa5e661 into master Oct 23, 2023
@DennisInSky DennisInSky deleted the dd/gr-exec-settings branch October 23, 2023 16:59
@shamilsan shamilsan added the B1-releasenotes The feature deserves to be added to the Release Notes label Oct 26, 2023
@shamilsan shamilsan changed the title feat(gstd, backend): Introduce gr_exec_settings sys-call feat(gstd, backend): Introduce gr_env_vars sys-call Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@techraed techraed techraed left review comments

@NikVolf NikVolf NikVolf approved these changes

@breathx breathx breathx approved these changes

@ark0f ark0f ark0f approved these changes

@shamilsan shamilsan Awaiting requested review from shamilsan

Assignees

@DennisInSky DennisInSky

Labels
A0-pleasereview PR is ready to be reviewed by the team A2-mergeoncegreen PR is ready to merge after CI passes B1-releasenotes The feature deserves to be added to the Release Notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@DennisInSky @NikVolf @ark0f @techraed @breathx @shamilsan