Skip to content
This repository has been archived by the owner on Mar 20, 2024. It is now read-only.

Initial serialization support #219

Merged
merged 2 commits into from Jul 7, 2023
Merged

Initial serialization support #219

merged 2 commits into from Jul 7, 2023

Conversation

brson
Copy link
Collaborator

@brson brson commented Jul 7, 2023

Work towards #128.

This is mostly-complete support for serialization and deserialization in the runtime. The patch is not perfect, and some types still do not work, but I am posting it to begin review, and possibly to merge it to unblock others' work.

As discussed on the issue, we don't necessarily need to use bcs, and because the available bcs crates don't quite fulfill our needs, I have instead used borsh here, which is mature, compact, and has a stable spec. We can open another issue about finalizing the serialization format before a release.

For testing purposes this adds a native function that can be named as std::bcs::test_from_bytes. This function is not part of the standard library, but when declared correctly is accessible to move code.

This required adding a second U256 type, called U256Placeholder here, in order to derive BorshSerialize/Deserialize against it. There are unsafe casts between vectors of the two u256 types. In the future we might use our own U256 type everywhere instead of splitting uses between ethnum::U256 and our own.

The only cases not completed are: vector<vector<T>>, vector<struct>, and reference. The vector cases I will complete soon. The reference case I imagine is unreachable and is marked as such.

Some test cases fail because of other errors in the compiler. They are commented out with a fixme.

@nvjle
Copy link

nvjle commented Jul 7, 2023

This required adding a second U256 type, called U256Placeholder here, in order to derive BorshSerialize/Deserialize against it. There are unsafe casts between vectors of the two u256 types. In the future we might use our own U256 type everywhere instead of splitting uses between ethnum::U256 and our own.

Makes sense for now. Ultimately, I'd rather use something standard like the crypto_bigint crate (no_std friendly) than either ethnum::U256 or homebrew.

The only cases not completed are: vector<vector<T>>, vector<struct>, and reference. The vector cases I will complete soon.

This is reasonable for now. FWIW, vector<struct> would be the best to implement first if you are prioritizing-- due to std:::option.

The reference case I imagine is unreachable and is marked as such.

I agree, this should be true for values ser/des to/from the persistent store. But I think we'll need these for arguments and return values of entry points. TBD.

Some test cases fail because of other errors in the compiler. They are commented out with a fixme.

The patch LGTM, ready to land.

BTW, I took a quick look at the generic compiler errors mentioned above:

  1. AssignKind::Store not yet implemented for signer. Easy fix which I'll attend to soon.
  2. Lack of move-native support for vector-of-struct comparison (or more fundamentally, just struct comparison). We currently assert in the compiler because it gives a more meaningful error message than crapping out in the runtime. Note that I have previously worked around this in the straight struct comparison case by generating LLVM IR to do the comparison. But for vector-of-struct and more generally, we'll need to do this in the runtime as part of vec_cmp_eq. That routine will currently todo!() for struct elements.

@brson
Copy link
Collaborator Author

brson commented Jul 7, 2023

I pushed the implementation for vector<vector<T>>, the test case for which also suffers from a missing eq implementation in the compiler. I'll land this as-is and then continue in another patch.

@nvjle
Copy link

nvjle commented Jul 7, 2023

I pushed the implementation for vector<vector<T>>, the test case for which also suffers from a missing eq implementation in the compiler. I'll land this as-is and then continue in another patch.

See also my comments above-- move-native is missing struct comparison and, by extension, vector-of-struct comparison, etc.

@brson brson merged commit 2a33713 into anza-xyz:llvm-sys Jul 7, 2023
4 checks passed
ksolana pushed a commit that referenced this pull request Jul 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants