Read and write xdr to file for persistent storage #24
Conversation
src/invoke.rs
Outdated
| } | ||
| }; | ||
|
|
||
| let mut lines = io::BufReader::new(file).lines(); |
There was a problem hiding this comment.
❗ Separating the XDR blobs with new lines, then splitting on those new lines here will probably result in corrupted XDR records. If a new line byte shows up anywhere in a ledger entry it'll be split into separate lines, and then decoded only in part which will error.
You actually don't need a delimiter between the entries. XDR entries have well defined lengths since all fields are either fixed length or variable length with a length prefix. So it is safe, to write each record to file, and then to simply read each record from the file one after the other.
You can still use a BufReader if you'd like, although you don't strictly need to.
There was a problem hiding this comment.
Oh, I should have mentioned, if you put all the ledger entries in a VecM<_, _> type that is in the stellar-xdr lib, then you can read and write them in a single call, without having to iterate over them, etc.
There was a problem hiding this comment.
Good point. I just removed the newline. I didn't go the VecM route because we have both LedgerKeys and LedgerEntries in the file.
src/invoke.rs
Outdated
|
|
||
| use clap::Parser; | ||
| use stellar_contract_env_host::{ | ||
| xdr::{Error as XdrError, ScVal, ScVec}, | ||
| im_rc::OrdMap, |
There was a problem hiding this comment.
It's fine if you want to use the im_rc::OrdMap, but in the CLI it is probably unnecessary to do so. You could use the stdlib's BTreeMap which retains order and doesn't require depending on any interesting dependencies.
There was a problem hiding this comment.
storage.map, which we pass to snapshot::commit, is an OrdMap, so it's seems simpler to use that everywhere instead of also including BTreeMap. What do you think?
| // LedgerEntry2 | ||
| // ... | ||
|
|
||
| // TODO: allow option to separate input and output file |
src/invoke.rs
Outdated
| // Initialize storage and host | ||
| // db_file format is one xdr object per line, where a LedgerKey is followed by the corresponding LedgerEntry. | ||
| // | ||
| // LedgerKey1 | ||
| // LedgerEntry1 | ||
| // LedgerKey2 | ||
| // LedgerEntry2 | ||
| // ... |
There was a problem hiding this comment.
Can you put this comment with the read / commit functions, since they're responsible for how that happens. You could put the functions inside a mod { ... } and put the comment there.
Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>
Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>
Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>
Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>
Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>
src/invoke.rs
Outdated
| #[clap(long, parse(from_os_str))] | ||
| snapshot_file: std::path::PathBuf, |
There was a problem hiding this comment.
One thing left, could we set a default value for this field? Ideally it is something as a default, like ledger.xdr.
What
Uses a file to store xdr LedgerKey-LedgerEntry pairs.
There are some TODO's in here that we should address next. I can also remove them and add issues instead.
Known limitations
This PR dumps xdr for now, but it's also possible to output JSON using Serde. I was able to get JSON working without without specifying custom serializers, but the output wasn't friendly. We would definitely benefit from spending some time specializing some of the XDR types with Serde. I have included the generic Serde output from the experiment I did, with ledger key on the first line, and the ledger entry on the second -