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

Implement ordered maps for smart contracts #405

Merged
merged 38 commits into from
Mar 27, 2024
Merged

Implement ordered maps for smart contracts #405

merged 38 commits into from
Mar 27, 2024

Conversation

limemloh
Copy link
Collaborator

@limemloh limemloh commented Mar 1, 2024 •
edited
Loading

Purpose

Closing #403

Changes

  • Add B-Tree based ordered maps where each node is stored separately in the low-level key-value store.

Checklist

  • My code follows the style of this project.
  • The code compiles without warnings.
  • I have performed a self-review of the changes.
  • I have documented my code, in particular the intent of the
    hard-to-understand areas.
  • (If necessary) I have updated the CHANGELOG.

@limemloh limemloh force-pushed the state-btree-map branch 2 times, most recently from 802e4f7 to 5dadfa5 Compare March 4, 2024 08:40
@limemloh limemloh marked this pull request as ready for review March 4, 2024 09:04
@limemloh limemloh requested review from abizjak, td202 and DOBEN March 4, 2024 09:05
td202
td202 reviewed Mar 5, 2024
View reviewed changes
Copy link
Contributor

@td202 td202 left a comment

Choose a reason for hiding this comment

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

Looks good. I'd like to see clearer documentation of the invariants. Ideally, I'd like to see some randomized testing, especially testing structural invariants (e.g. the ordering of keys and size of nodes), and a relational invariant with respect to a model (e.g. std::collections::BTreeSet).

concordium-std/src/types.rs Outdated Show resolved Hide resolved
concordium-std/src/types.rs Outdated Show resolved Hide resolved
concordium-std/src/types.rs Outdated Show resolved Hide resolved
concordium-std/src/types.rs Outdated Show resolved Hide resolved
concordium-std/src/types.rs Outdated Show resolved Hide resolved
concordium-std/src/impls.rs Outdated Show resolved Hide resolved
concordium-std/src/impls.rs Outdated Show resolved Hide resolved
concordium-std/src/impls.rs Outdated Show resolved Hide resolved
concordium-std/src/impls.rs Outdated Show resolved Hide resolved
concordium-std/src/impls.rs Outdated Show resolved Hide resolved
DOBEN
DOBEN reviewed Mar 6, 2024
View reviewed changes
concordium-std/CHANGELOG.md Outdated Show resolved Hide resolved
concordium-std/src/test_infrastructure.rs Outdated Show resolved Hide resolved
concordium-std/src/test_infrastructure.rs Outdated Show resolved Hide resolved
concordium-std/src/types.rs Outdated Show resolved Hide resolved
concordium-std/src/types.rs Outdated Show resolved Hide resolved
concordium-std/src/types.rs Outdated Show resolved Hide resolved
concordium-std/src/impls.rs Outdated Show resolved Hide resolved
concordium-std/src/impls.rs Outdated Show resolved Hide resolved
concordium-std/src/impls.rs Outdated Show resolved Hide resolved
This was referenced Mar 11, 2024
Merged
Merged
abizjak
abizjak reviewed Mar 11, 2024
View reviewed changes
concordium-std/CHANGELOG.md Outdated Show resolved Hide resolved
concordium-std/src/impls.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/types.rs Outdated Show resolved Hide resolved
@limemloh
Copy link
Collaborator Author

I have asked for a review again.
Currently, the only remaining task for this PR is to have the CI run the internal wasm unit-tests, but this is blocked until a new version of cargo-concordium is released.

@abizjak
Copy link
Contributor

abizjak commented Mar 21, 2024

I have asked for a review again. Currently, the only remaining task for this PR is to have the CI run the internal wasm unit-tests, but this is blocked until a new version of cargo-concordium is released.

You can just build a binary now before it is released and download it. We don't want to be building cargo-concordium every time anyhow.

limemloh added 2 commits March 22, 2024 13:33
@limemloh
Add guide for choosing the right collection (#409)
29de383 
* Document when to use StateMap and StateBTreeMap

* Address review comments
@limemloh
Update rust version and fix clippy
b4f08f8
@limemloh limemloh force-pushed the state-btree-map branch 2 times, most recently from 6b5e6b5 to 00c85d4 Compare March 24, 2024 18:39
abizjak
abizjak reviewed Mar 25, 2024
View reviewed changes
concordium-std/Cargo.toml Show resolved Hide resolved
concordium-std/src/types.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/impls.rs Outdated Show resolved Hide resolved
concordium-std/src/impls.rs Show resolved Hide resolved
concordium-std/src/state_btree.rs Show resolved Hide resolved
concordium-std/src/state_btree.rs Show resolved Hide resolved
@limemloh limemloh requested a review from abizjak March 25, 2024 12:55
abizjak
abizjak approved these changes Mar 25, 2024
View reviewed changes
.github/workflows/linter.yml Outdated Show resolved Hide resolved
.github/workflows/linter.yml Outdated Show resolved Hide resolved
concordium-std/src/impls.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
@limemloh limemloh force-pushed the state-btree-map branch 5 times, most recently from 6843d47 to 2bb8764 Compare March 26, 2024 08:31
DOBEN
DOBEN approved these changes Mar 26, 2024
View reviewed changes
.github/workflows/linter.yml Outdated Show resolved Hide resolved
concordium-std/Cargo.toml Outdated Show resolved Hide resolved
concordium-std/src/impls.rs Outdated Show resolved Hide resolved
concordium-std/src/impls.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Show resolved Hide resolved
concordium-std/src/state_btree.rs Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
td202
td202 approved these changes Mar 27, 2024
View reviewed changes
Copy link
Contributor

@td202 td202 left a comment

Choose a reason for hiding this comment

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

Looks good!

concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Outdated Show resolved Hide resolved
concordium-std/src/state_btree.rs Show resolved Hide resolved
@abizjak abizjak merged commit 670ffab into main Mar 27, 2024
148 checks passed
@abizjak abizjak deleted the state-btree-map branch March 27, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DOBEN DOBEN DOBEN approved these changes

@td202 td202 td202 approved these changes

@abizjak abizjak abizjak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@limemloh @abizjak @td202 @DOBEN