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

[framework] Adds bcs::peel_enum_tag #20984

Merged
merged 2 commits into from
Feb 3, 2025
Merged

[framework] Adds bcs::peel_enum_tag #20984

merged 2 commits into from
Feb 3, 2025

Conversation

damirka
Copy link
Contributor

@damirka damirka commented Jan 25, 2025

Description

Minor improvement for bcs library, helper to parse enums.

Test plan

Features tests.


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.

  • Protocol: Changes to the Sui framework, introduction of bcs::peel_enum_tag function
  • Nodes (Validators and Full nodes):
  • gRPC:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:

@damirka damirka added the move label Jan 25, 2025
@damirka damirka self-assigned this Jan 25, 2025
Copy link

vercel bot commented Jan 25, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 3, 2025 3:35pm
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Feb 3, 2025 3:35pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Feb 3, 2025 3:35pm

Copy link
Contributor

@tnowacki tnowacki left a comment

Choose a reason for hiding this comment

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

I am thinking that this shouldn't really be a macro after some further reading.
peel_vec and peel_option both have a return type of their constructing type--vector and Option respectively.

But peel_enum here is just passing back the return value of peel_enum_tag.

For example

public macro fun peel_enum<$T>($bcs: &mut BCS, $f: |u64| -> $T): $T
...
bcs.peel_enum!(|tag| match (tag) {
    0 => Enum::Empty,
    1 => Enum::U8(bcs.peel_u8()),
    2 => Enum::U16(bcs.peel_u16()),
    _ => abort,
}

could just be

public fun peel_enum_tag(bcs: &mut BCS): u64
...
match (peel_enum_tag(tag)) {
    0 => Enum::Empty,
    1 => Enum::U8(bcs.peel_u8()),
    2 => Enum::U16(bcs.peel_u16()),
    _ => abort,
}

TLDR the macro is just function application, which isn't really that interesting I think?

@damirka damirka merged commit d719180 into main Feb 3, 2025
50 of 51 checks passed
@damirka damirka deleted the ds/bcs-peel-enum branch February 3, 2025 16:25
@damirka damirka changed the title [framework] Adds peel_enum to bcs [framework] Adds bcs::peel_enum_tag Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants