Skip to content
This repository has been archived by the owner on Jun 12, 2018. It is now read-only.

MUL-1227: signing serialize golos transaction #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PashaKlybik
Copy link
Member

No description provided.

Copy link
Contributor

@Enmk Enmk left a comment

Choose a reason for hiding this comment

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

Please fix mentioned issues + provide more descriptive commit message.

Also it would make sense to enable Golos transaction tests, to verify that this implementation does what it is supposed to do.

@@ -442,6 +548,7 @@ BinaryDataPtr GolosTransaction::serialize()
encode(*m_signature, CODEC_HEX).c_str()
);

std::cerr << buffer;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this.

}

template <typename T>
GolosBinaryStream& write_as_binary(const T& data, GolosBinaryStream& stream)
Copy link
Contributor

Choose a reason for hiding this comment

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

Stream should be passed by pointer here and other non-operator<< functions.


GolosBinaryStream& operator<<(GolosBinaryStream& stream, const std::time_t& time)
{
stream.write_data(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that intentional to omit explicit endianess of the time ?

temp.insert(0, token_name);
temp.resize(7);
*stream << temp;
*stream << static_cast<uint8_t>(memo.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we have a certain pattern of writing a string to a BinaryStream here, and it makes sense to move that to appropriate operator<<

{
*stream << (uint8_t)0x02; // transfer_operation id
Copy link
Contributor

Choose a reason for hiding this comment

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

Make a named const out of this 0x02


namespace multy_core
{
namespace internal
{

const BinaryDataPtr GOLOS_CHAIN_ID = decode("782a3039b478c839e4cb0c941ff4eaeb7df40bdd68bd441afd444b9da763de12", CODEC_HEX);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't do that, you should make an array with bytes corresponding to this.

*stream << amount.get_value_as_uint64();
*stream << static_cast<uint8_t>(GOLOS_VALUE_DECIMAL_PLACES);

std::string temp (7, '\0');
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks silly, could you please explain what 7 stands for? also, this could be done much easier:

std::string normalized_token_name(token_name);
normalized_token_name.resize(7);

: TransactionBase(blockchain_type),
GolosTransaction::GolosTransaction(const Account& account)
: TransactionBase(account.get_blockchain_type()),
m_account(account),
Copy link
Contributor

Choose a reason for hiding this comment

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

Account instance should not be locked for lifetime of transaction, please store the private key instead or add a transaction-level PropertyT for private key.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants