[framework] Add native function to check protocol feature flags in Move #24422
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
lxfind
force-pushed
the
xun/check-feature-flag-in-move
branch
from
6206719 to
31ef896
Compare
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
lxfind
force-pushed
the
xun/check-feature-flag-in-move
branch
from
31ef896 to
5adec6c
Compare
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
lxfind
force-pushed
the
xun/check-feature-flag-in-move
branch
from
5adec6c to
67b817c
Compare
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
tnowacki
left a comment
tnowacki
left a comment
There was a problem hiding this comment.
Could you explain where you plan on using this? Previously we haven't needed any of this since we just shove this behavior in the native itself
|
I would like to gate object balance withdraw separately from address balance withdraw |
lxfind
force-pushed
the
xun/check-feature-flag-in-move
branch
from
67b817c to
9432523
Compare
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
But both of those bottom out in a native function? Why not just gate within the native like we are doing currently? |
|
To be clear, I don't inherently dislike this PR by any means, it just seems like overkill unless we really need it. |
|
It's not though: https://github.com/MystenLabs/sui/blob/main/crates/sui-framework/packages/sui-framework/sources/funds_accumulator.move#L86 I heard from Mark that he has wanted this a few times, but since there are always walkarounds (some not so ideal) so this wasn't ever a big complaint. |
| #[allow(unused_const)] | ||
| #[error(code = 0)] | ||
| const EFeatureFlagNotFound: vector<u8> = b"Feature flag not found"; |
There was a problem hiding this comment.
@tzakian how does this work with natives? (sorry should have added this here instead of the other PR)
There was a problem hiding this comment.
Hmmm I'd err on not using a clever error for native errors as they can't really raise clever errors (since in particular there is no line number to report).
There was a problem hiding this comment.
It makes me a bit sad though 😢 but yea I think the best thing to do here is just a normal u64 constant error code.
There was a problem hiding this comment.
I have removed all the errors and now they are just invariant violations
tnowacki
left a comment
tnowacki
left a comment
There was a problem hiding this comment.
Overall LGTM, I just want to think through the error case: Both for clever errors, and for invariant violations.
I think it would be safe to just invariant violation on unknown/invalid strings? But maybe there is something tricky going on with upgrades?
| Err(_) => { | ||
| return Ok(NativeResult::err( |
There was a problem hiding this comment.
I wonder if we should just make this an invariant violation in some way (similar with the error below)
There was a problem hiding this comment.
Yeah this should not happen unless we expose this to user packages.
There was a problem hiding this comment.
Made them invariants
lxfind
force-pushed
the
xun/check-feature-flag-in-move
branch
from
9432523 to
4f0548d
Compare
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
Adds sui::protocol_config::is_feature_enabled() to allow Move code to query protocol feature flags at runtime. This enables conditional logic and fallbacks based on protocol version capabilities.
lxfind
force-pushed
the
xun/check-feature-flag-in-move
branch
from
4f0548d to
cdca8f2
Compare
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
Adds sui::protocol_config::is_feature_enabled() to allow Move code to query protocol feature flags at runtime. This enables conditional logic and fallbacks based on protocol version capabilities.
Description
Describe the changes or additions included in this PR.
Test plan
How did you test the new or updated feature?
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.