Skip to content

Experiment with ifdef-guarded changes#294

Draft
leighmcculloch wants to merge 2 commits intocurrfrom
experiment-with-ifdef
Draft

Experiment with ifdef-guarded changes#294
leighmcculloch wants to merge 2 commits intocurrfrom
experiment-with-ifdef

Conversation

@leighmcculloch
Copy link
Member

What

Experiment with ifdef-guarded changes.

Why

For:

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR experiments with introducing an optional SCV_SPARSE_MAP variant into the Soroban SCVal XDR by conditionally compiling enum/union members behind a SPARSE_MAP preprocessor define.

Changes:

  • Adds SCV_SPARSE_MAP to SCValType when SPARSE_MAP is defined.
  • Adds a corresponding SCV_SPARSE_MAP arm to the SCVal union when SPARSE_MAP is defined.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 73 to 78
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

Using #ifdef SPARSE_MAP inside the XDR enum makes the numeric type space dependent on compile-time flags. This can lead to incompatible serialized data between components built with different defines (e.g., one side knows SCV_SPARSE_MAP and the other doesn’t). Prefer keeping the XDR schema deterministic (e.g., define the new enum value unconditionally or introduce a separate, versioned XDR type) rather than CPP-guarding enum members.

Copilot uses AI. Check for mistakes.
Comment on lines 294 to 297
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The SCV_SPARSE_MAP union arm is also CPP-guarded. If one producer is built with SPARSE_MAP and emits type 22, a consumer built without it will fail to decode the SCVal union. Consider keeping the union arm present (even if unused) to maintain wire compatibility across builds, or move this experiment behind protocol-versioned/explicit feature negotiation instead of preprocessor guards in XDR.

Copilot uses AI. Check for mistakes.

#ifdef SPARSE_MAP
case SCV_SPARSE_MAP:
SCMap *sparseMap;
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

Field name sparseMap uses camelCase, while other SCVal union arms use snake_case (e.g., nonce_key) or short identifiers (vec, map). Rename to a consistent style (e.g., sparse_map) to match the surrounding XDR conventions and generated bindings.

Suggested change
SCMap *sparseMap;
SCMap *sparse_map;

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments