Skip to content
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

Fix #162: allow empty topic list in messages #164

Merged
merged 3 commits into from
Apr 17, 2015
Merged

Fix #162: allow empty topic list in messages #164

merged 3 commits into from
Apr 17, 2015

Conversation

karalabe
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 94.24% when pulling 6a058b6 on karalabe:empty-topics-fix into 271b00f on ethereum:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 94.24% when pulling 6a058b6 on karalabe:empty-topics-fix into 271b00f on ethereum:develop.

@frozeman
Copy link
Contributor

The fix should actually not be required.

In the inputPostFormatter it will already create an topics array and even accept strings, which will then get add as the first topic.

the outputPostFormatter assumes that there is always an topic array coming, which the node should send.

@@ -159,6 +159,9 @@ var inputPostFormatter = function(post) {
post.priority = utils.fromDecimal(post.priority);

// fallback
if (post.topics == undefined) {
post.topics = []
}
if (!utils.isArray(post.topics)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this? As the line below already takes care of creating and array if non is given.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remove this check, then the sample code crashes with

TypeError: Cannot read property 'length' of undefined
    at toHexNative (/usr/lib/node_modules/web3/lib/utils/utils.js:121:27)
    at Object.fromAscii (/usr/lib/node_modules/web3/lib/utils/utils.js:139:15)
    at /usr/lib/node_modules/web3/lib/web3/formatters.js:168:22
    at Array.map (native)
    at inputPostFormatter (/usr/lib/node_modules/web3/lib/web3/formatters.js:167:31)
    at /usr/lib/node_modules/web3/lib/web3/method.js:86:28
    at Array.map (native)
    at Method.formatInput (/usr/lib/node_modules/web3/lib/web3/method.js:85:32)
    at Method.toPayload (/usr/lib/node_modules/web3/lib/web3/method.js:130:23)
    at Method.send (/usr/lib/node_modules/web3/lib/web3/method.js:148:24)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the contents of post.topics: [ undefined ] if I don't specify any in my code.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, can you refactor the part like this:

    // fallback
    if (!utils.isArray(post.topics)) {
        post.topics = post.topics ? [post.topics] : [];
    }

    // format the following options
    post.topics = post.topics.map(function(topic){
        return utils.fromAscii(topic);
    });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure :)

frozeman added a commit that referenced this pull request Apr 17, 2015
Fix #162: allow empty topic list in messages
@frozeman frozeman merged commit 135b044 into web3:develop Apr 17, 2015
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 94.34% when pulling b326202 on karalabe:empty-topics-fix into 271b00f on ethereum:develop.

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

Successfully merging this pull request may close these issues.

None yet

3 participants