Skip to content

feat: create bounded vector for sign request args#2165

Open
DSharifi wants to merge 8 commits intomainfrom
dsharifi/add-bounded-vec
Open

feat: create bounded vector for sign request args#2165
DSharifi wants to merge 8 commits intomainfrom
dsharifi/add-bounded-vec

Conversation

@DSharifi
Copy link
Copy Markdown
Contributor

@DSharifi DSharifi commented Feb 18, 2026

closes #2186

@DSharifi DSharifi changed the title chore: create bounded vector for transport types chore: create bounded vector for sign request args Feb 19, 2026
@DSharifi DSharifi force-pushed the dsharifi/add-bounded-vec branch from dddaec2 to 7d172e1 Compare February 20, 2026 08:55
Comment on lines -31 to -46
schemars(with = "[u8; 32]"),
borsh(schema(with_funcs(
declaration = "<[u8; 32] as ::borsh::BorshSchema>::declaration",
definitions = "<[u8; 32] as ::borsh::BorshSchema>::add_definitions_recursively"
),))
schemars(
with = "hex_serde::HexString<ECDSA_PAYLOAD_SIZE_BYTES, ECDSA_PAYLOAD_SIZE_BYTES>"
)
)]
Bytes<32, 32>,
BoundedVec<u8, ECDSA_PAYLOAD_SIZE_BYTES, ECDSA_PAYLOAD_SIZE_BYTES>,
),
Eddsa(
#[serde(with = "hex_serde")]
#[cfg_attr(
all(feature = "abi", not(target_arch = "wasm32")),
schemars(with = "Vec<u8>"),
borsh(schema(with_funcs(
declaration = "<Vec<u8> as ::borsh::BorshSchema>::declaration",
definitions = "<Vec<u8> as ::borsh::BorshSchema>::add_definitions_recursively"
),))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There was previously a bug in the schema definition. We expect a hex string, but declared sequence of bytes (vector)

Comment on lines +128 to +141
#[derive(Debug, BorshSerialize, BorshDeserialize)]
struct StaleData {
pending_signature_requests_pre_upgrade: LookupMap<v3_4_1_state::SignatureRequest, YieldIndex>,
}

impl StaleData {
fn new() -> Self {
Self {
pending_signature_requests_pre_upgrade: LookupMap::new(
StorageKey::PendingSignatureRequestsV2,
),
}
}
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This migration step is needed because the new BoundedVec deosn't have idential borsh/hash implementation.

Copy link
Copy Markdown
Contributor Author

@DSharifi DSharifi Feb 20, 2026

Choose a reason for hiding this comment

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

Most of the implementation is taken from https://github.com/ergoplatform/bounded-vec/blob/develop/src/bounded_vec.rs (license allows this).

Some small changes have been made, such as not implementing AsMut since that can break the invariants.

Borsh and scehma implementations have also been added for ABI generation.

Comment on lines 2616 to 2642
@@ -2633,12 +2638,7 @@ expression: abi
],
"properties": {
"Eddsa": {
"type": "array",
"items": {
"type": "integer",
"format": "uint8",
"minimum": 0.0
}
"$ref": "#/definitions/HexString_Min32_Max1232"
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There was previously a bug in the schema definition. We expect a hex string, but declared sequence of bytes (vector) in the schema

@DSharifi DSharifi marked this pull request as ready for review February 20, 2026 10:24
@claude
Copy link
Copy Markdown

claude bot commented Feb 20, 2026

PR title type suggestion: This PR introduces a new bounded collections library with substantial new functionality (~2,000 lines of code in a new crate) integrated throughout the codebase for type-safe collection handling. The type prefix should probably be feat: instead of chore:

Suggested title: feat: create bounded collections for compile-time size validation

@claude
Copy link
Copy Markdown

claude bot commented Feb 20, 2026

PR title type suggestion: This PR introduces new crates and types (bounded-collections, non-empty-collections) with API changes and functional impact across the system, so the type prefix should probably be 'feat:' instead of 'chore:'.

Suggested title: 'feat: create bounded vector and non-empty collections for sign request args and foreign chains'

@DSharifi DSharifi changed the title chore: create bounded vector for sign request args feat: create bounded vector for sign request args Feb 20, 2026
@DSharifi
Copy link
Copy Markdown
Contributor Author

@claude please review

@claude
Copy link
Copy Markdown

claude bot commented Feb 20, 2026

PR Review: chore: create bounded vector for sign request args

I reviewed the full diff focusing on correctness, migration safety, and backward compatibility.

Summary

This PR replaces the ad-hoc Bytes<MIN_LEN, MAX_LEN> type with a general-purpose BoundedVec<T, L, U, W> for signature payloads, renames non-empty-collections to bounded-collections, and handles the on-chain state migration correctly by:

  1. Moving old pending_signature_requests (V2 storage key) into StaleData during migration
  2. Using a new V3 storage key for new requests
  3. Adding fallback lookups/removals for in-flight pre-upgrade requests

Critical Issues Found

None. The migration strategy is sound:

  • Borsh compatibility: Both old Bytes(Vec<u8>) and new BoundedVec { inner: Vec<u8> } serialize identically via borsh (both delegate to Vec<u8>::serialize). Enum discriminants are preserved (same variant order).
  • Storage key separation: V2 key is preserved for stale data, V3 for new requests — no collision.
  • Fallback paths: get_pending_request, respond_signature, and return_signature_and_clean_state_on_success all correctly fall back to the pre-upgrade stale map.
  • Tests: New unit tests cover the stale-map fallback paths for timeout and success cases.

Minor Observations (non-blocking)

  • hex_serde::HexString::json_schema (bounded_vec.rs:799-800) uses (L * 2) as u32 and (U * 2) as u32 which could theoretically overflow for very large bounds. Not an issue with current usage (max 1232), but u32::try_from(L.checked_mul(2).unwrap_or(usize::MAX)) would be more defensive. This is ABI-only code so not a production concern.

✅ Approved — the migration is well-structured with proper fallback handling for in-flight requests.

@gilcu3
Copy link
Copy Markdown
Contributor

gilcu3 commented Feb 28, 2026

Superseded by #2199

@gilcu3 gilcu3 closed this Feb 28, 2026
@DSharifi
Copy link
Copy Markdown
Contributor Author

DSharifi commented Mar 1, 2026

Superseded by #2199

No, that PR did not close #2186

@DSharifi DSharifi reopened this Mar 1, 2026
@gilcu3
Copy link
Copy Markdown
Contributor

gilcu3 commented Mar 1, 2026

Superseded by #2199

No, that PR did not close #2186

Then we should re-open #2186, it is closed now

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.

create a bounded vector type

2 participants