-
Notifications
You must be signed in to change notification settings - Fork 25
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
sub-chain #330
Conversation
@@ -92,14 +97,34 @@ namespace eosio | |||
auto s = reinterpret_cast<const char*>(src); | |||
data.insert(data.end(), s, s + sz); | |||
} | |||
void write_str(std::string_view s) { write(s.data(), s.size()); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to make this a non-member, so that it doesn't need to be duplicated for every stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
struct vector_stream | ||
struct stream_base | ||
{ | ||
static constexpr bool time_point_include_z = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think information about how to format a specific type belongs in the generic stream interface.
I would suggest something along the lines of:
template<typename S>
constexpr bool time_point_include_z(const S&) { return false; }
- This can be defined alongside the definition of
to_json
for time points - The same method can be used to allow configuration for any type without modifying the base streams.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
} | ||
|
||
template <typename F, typename Arg, typename... Args, typename... Names> | ||
void for_each_named_type(F&& f, type_list<Arg, Args...>, const char* name, Names... names) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be implemented with a fold-expression without using recursion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
struct member_fn; | ||
|
||
template <typename R, typename T, typename... Args> | ||
struct member_fn<R (T::*)(Args...)> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These specializations are far from complete, of course...
https://www.boost.org/libs/callable_traits/doc/html/index.html#callable_traits.introduction.motivation
uint32_t num = 0; | ||
eosio::checksum256 previous; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this duplicate members that are already in the eosioBlock
field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The subchain blocks form a new chain:
- Sequentially increasing block num, starting at 1, with no gaps
- Each block references the previous block's hash
Each block contains a portion of an eosio block; those blocks skip.
|
||
const block_with_id* block_by_num(uint32_t num) const | ||
{ | ||
auto it = std::lower_bound(blocks.begin(), blocks.end(), num, by_block_num); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since add_block
requires the blocks to have sequential block numbers, a binary search is a waste of time. You can just calculate the index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
} | ||
|
||
template <typename Connection, | ||
typename Key, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably worth noting that for the cursor to work correctly, the container cannot hold multiple equivalent keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note added
[[clang::import_name("eosio_assert_message"), noreturn]] void | ||
eosio_assert_message(uint32_t, const char* msg, uint32_t len) | ||
{ | ||
polyfill::abort_message(msg, len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not checking the condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wrappers only call it with a false condition, plus expect it to have [[noreturn]]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished my 1st review round. This is so great! I'm super excited with the history possibilities that we are going to have now!
I looked at the pipeline build and the js side (box, common and example).
On top of that:
- Could you please add README instructions on box of what is needed to run the history websocket endpoint?
- Also an option on the box to run it without any history server, do we have that already and I missed?
- Add README instructions on the example history app of how to run it with, it requires an eden box with the history flag on running somewhere (pointing to box readme in case the user wants to run locally).
Added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the web side and pipeline lgtm! this is awesome!!
No description provided.