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

MUL-1227: signing serialize golos transaction#88

Open
PashaKlybik wants to merge 1 commit intoAppscrunch:masterfrom
PashaKlybik:MUL-1227-sign-golos-Tx
Open

MUL-1227: signing serialize golos transaction#88
PashaKlybik wants to merge 1 commit intoAppscrunch:masterfrom
PashaKlybik:MUL-1227-sign-golos-Tx

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.

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<<

void write_to_stream(GolosBinaryStream* /*stream*/) const override
void write_to_stream(GolosBinaryStream* stream) const override
{
*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 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