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

DOCS: Rework Portable storage format example #8323

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

jeffro256
Copy link
Contributor

@jtgrassie pointed out that the example I provided was colored incorrectly. He also made the good point that the image wasn't easy to review/correct. I reworked the example so that it's text-only. It's easier to review and edit, and reveals the structure better in my opinion. Also this is easier for people who can't distinguish colors as easily.

Make sure to double-check this work because there's a decent chance I screwed up the comments. The actual byte data was generated and should be solid. I'm also very open to suggestions about changing the style or structure.

@jeffro256 jeffro256 marked this pull request as draft May 11, 2022 18:42
@jeffro256 jeffro256 force-pushed the rework_storage_example branch from 1b51a00 to f50601f Compare June 21, 2022 03:23
@jtgrassie
Copy link
Contributor

Why is this in DRAFT status? It needs merging as the current doc is inaccurate.

@jeffro256
Copy link
Contributor Author

It needs merging as the current doc is inaccurate.

I agree, I was waiting to see if anyone was going to double-check my work, since posting a example that is wrong twice would be embarrassing. Would you be willing to?

@jeffro256 jeffro256 marked this pull request as ready for review July 21, 2022 21:35
```
01 11 01 01 01 01 02 01 // Signature
01 // Version
14 // Varint size of section (5)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not the size of the section, it's the count of section entries.

Copy link
Contributor

Choose a reason for hiding this comment

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

The rest looks reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated PR with your suggestions. Thanks!

@jtgrassie pointed out that the example I provided was colored incorrectly. He also made the good point that the image wasn't easy to review/correct. I reworked the example so that it's text-only. It's easier to review and edit, and reveals the structure better in my opinion. Also this is easier for people who can't distinguish colors as easily.

Make sure to double-check this work because there's a decent chance I screwed up the comments. The actual byte data was generated and should be solid.
@jeffro256 jeffro256 force-pushed the rework_storage_example branch from f50601f to 564fa30 Compare July 22, 2022 17:20
@jeffro256
Copy link
Contributor Author

@selsta Before you merge, I am going to double check one thing with portable storage format: bool arrays. In c++ there is a specialization for std::vector<bool> which packs each bool value into a bit instead of the normal byte. I want to make double check the behavior of serializing a vector of bools as a blob matches with the code I used to generate the example bytes.

@selsta
Copy link
Collaborator

selsta commented Jul 24, 2022

@jeffro256 ok, please comment here when I can re-add it to the merge queue.

@jeffro256
Copy link
Contributor Author

@selsta Okay we're good!

@jeffro256
Copy link
Contributor Author

You can double check results with this code:

#include <iostream>
#include <vector>

#include "byte_slice.h"
#include "hex.h"
#include "serialization/keyvalue_serialization.h"
#include "span.h"
#include "storages/portable_storage.h"

using namespace epee;

struct Data {

	std::vector<bool> ba;

	BEGIN_KV_SERIALIZE_MAP()
		KV_SERIALIZE(ba)
	END_KV_SERIALIZE_MAP()

};


int main() {
	serialization::portable_storage ps;
	const Data data = { { true, false, true, true, false } };

	data.store(ps);

	byte_slice bs;
	ps.store_to_binary(bs);

	span<const uint8_t> byte_span(bs.data(), bs.size());

	std::cout << to_hex::string(byte_span) << std::endl;

	return 0;
}

Put easylogging++ and epee folders in current directory, then compile with: g++ -I epee/include/ -I easylogging++/ -D AUTO_INITIALIZE_EASYLOGGINGPP -o ps_ba main.cpp easylogging++/easylogging++.cc epee/src/hex.cpp epee/src/byte_slice.cpp epee/src/portable_storage.cpp epee/src/int-util.cpp epee/src/memwipe.c epee/src/wipeable_string.cpp epee/src/parserse_base_utils.cpp epee/src/byte_stream.cpp

Produces output: 011101010101020101040262618b140100010100

@jtgrassie
Copy link
Contributor

I am going to double check one thing with portable storage format: bool arrays. In c++ there is a specialization for std::vector<bool> which packs each bool value into a bit instead of the normal byte. I want to make double check the behavior of serializing a vector of bools as a blob matches with the code I used to generate the example bytes.

This is merely an example of why I wrote the following in the original document:

It's important to understand that entry values can be encoded any way in which an implementation chooses.

(and also why I don't see the value of the JSON example)

@luigi1111 luigi1111 merged commit c286e03 into monero-project:master Aug 23, 2022
@jeffro256 jeffro256 deleted the rework_storage_example branch October 31, 2023 07:23
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.

4 participants