Unity build: prepare support in irohad #238
Conversation
lebdron
force-pushed
the
refactor/unity-build-prepare
branch
2 times, most recently
from
7096c5d
to
fdbb2ad
Compare
shared_model/backend/protobuf/impl/deserialize_repeated_transactions.cpp
Outdated
Show resolved
Hide resolved
shared_model/backend/protobuf/impl/deserialize_repeated_transactions.cpp
Outdated
Show resolved
Hide resolved
irohad/multi_sig_transactions/transport/impl/mst_transport_grpc.cpp
Outdated
Show resolved
Hide resolved
| log_->warn("Transaction deserialization failed: hash {}, {}", | ||
| e->error.hash, | ||
| e->error.error); | ||
| return ::grpc::Status::OK; |
There was a problem hiding this comment.
it seems not OK at all. Also in other similar cases.
There was a problem hiding this comment.
Do we still use transport error codes for business logic?
|
|
||
| iroha::expected::Result<interface::types::SharedTxsCollectionType, | ||
| TransactionFactoryType::Error> | ||
| deserializeTransactions( |
There was a problem hiding this comment.
it seems OK for this PR just to speed up the merge, but apart from that, I think that this approach is wrong, because the task it solves is not constrained to proto transactions. I would solve it with something like
namespace expected {
template<typename V, typename E>
Result<std::vector<V>, E> allValuesOrFirstError(std::vector<Result<V, E>> results) {
std::vector<V> values;
for (auto &result : results) {
if (auto e = resultToOptionalError(result)) {
return e.value();
}
values.emplace_back(resultToOptionalValue(std::move(result)).value());
}
return values;
}
}There was a problem hiding this comment.
There is no need to generalize until there are at least two places where it can be reused. Templates are cool, but it is better to avoid them if possible
There was a problem hiding this comment.
when there will be a second place, this piece will most likely not be reused
There was a problem hiding this comment.
also I think that there are places besides this pr, where the proposed code can be used.
| log_->warn("Transaction deserialization failed: hash {}, {}", | ||
| e->error.hash, | ||
| e->error.error); | ||
| return ::grpc::Status::OK; |
There was a problem hiding this comment.
Do we still use transport error codes for business logic?
lebdron
force-pushed
the
refactor/unity-build-prepare
branch
from
6607dcf
to
d966dab
Compare
Network transaction endpoints: remove code duplication and make more strict validation Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
lebdron
force-pushed
the
refactor/unity-build-prepare
branch
from
d966dab
to
fcb68b4
Compare
Description of the Change
Benefits
Potential support for unity build of irohad
Possible Drawbacks
None