Skip to content

Store experimental flag in metadata CBOR#2712

Merged
axic merged 4 commits intodevelopfrom
experimental-metadata
Aug 11, 2017
Merged

Store experimental flag in metadata CBOR#2712
axic merged 4 commits intodevelopfrom
experimental-metadata

Conversation

@axic
Copy link
Contributor

@axic axic commented Aug 8, 2017

Depends on #2690 and #2704.

@axic axic force-pushed the experimental-metadata branch 4 times, most recently from b1f2aba to d12c138 Compare August 10, 2017 16:19
@axic axic requested a review from chriseth August 10, 2017 16:19
@axic axic force-pushed the experimental-metadata branch 2 times, most recently from bea518c to ab84520 Compare August 10, 2017 16:58
chriseth
chriseth reviewed Aug 11, 2017
View reviewed changes
libsolidity/interface/CompilerStack.cpp Outdated

namespace
{
bool safeExperimentalFeature(set<ExperimentalFeature> features)
Copy link
Contributor

@chriseth chriseth Aug 11, 2017

Choose a reason for hiding this comment

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

const&

Copy link
Contributor Author

@axic axic Aug 11, 2017

Choose a reason for hiding this comment

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

const& for a bool?

error: returning reference to local temporary object

Copy link
Contributor

@chriseth chriseth Aug 11, 2017

Choose a reason for hiding this comment

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

No, for the set

Copy link
Contributor Author

@axic axic Aug 11, 2017

Choose a reason for hiding this comment

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

Added.

chriseth
chriseth reviewed Aug 11, 2017
View reviewed changes
libsolidity/interface/CompilerStack.cpp Outdated

namespace
{
bool safeExperimentalFeature(set<ExperimentalFeature> features)
Copy link
Contributor

@chriseth chriseth Aug 11, 2017

Choose a reason for hiding this comment

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

also naming: activatesSomeUnsafeFeature, onlySafeFeaturesActivated?

chriseth
chriseth reviewed Aug 11, 2017
View reviewed changes
libsolidity/interface/CompilerStack.cpp Outdated
if (!safeExperimentalFeature(_contract.sourceUnit().annotation().experimentalFeatures))
cborEncodedMetadata =
// CBOR-encoding of {"bzzr0": dev::swarmHash(metadata), "experimental": true}
bytes{0xa2, 0x65, 'b', 'z', 'z', 'r', '0', 0x58, 0x20} +
Copy link
Contributor

@chriseth chriseth Aug 11, 2017

Choose a reason for hiding this comment

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

indentation

Copy link
Contributor

@chriseth chriseth Aug 11, 2017

Choose a reason for hiding this comment

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

Also, can you extract the common part?

Copy link
Contributor Author

@axic axic Aug 11, 2017

Choose a reason for hiding this comment

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

Common part of cborEncodedMetadata? Note the first byte is different too.

Copy link
Contributor

@chriseth chriseth Aug 11, 2017

Choose a reason for hiding this comment

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

yes, so basically everything apart from the first byte. CBOR is defined in a way so that it can be composed at byte-level, isn't it?

Copy link
Contributor Author

@axic axic Aug 11, 2017

Choose a reason for hiding this comment

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

Yes and no. 0xa1 means hash map with one element, 0x65 b z ... is the key and the hash is the value.

I could pull out the key+value into a separate bytes variable, which is fine for this piece of code, but won't make it valid CBOR elements :)

@axic axic force-pushed the experimental-metadata branch 3 times, most recently from 12f5013 to 6525da8 Compare August 11, 2017 11:17
@axic
Copy link
Contributor Author

axic commented Aug 11, 2017 •
edited
Loading 

experimental.sol:1:1: Warning: Experimental features are turned on. Do not use experimental features on live deployments.
pragma experimental __test;
^-------------------------^
experimental.sol:2:1: Error: Duplicate experimental feature name.
pragma experimental __test;
^-------------------------^

None of the tests helpers CHECK_ERROR, CHECK_ERROR_ALLOW_MULTI, CHECK_WARNING_ALLOW_MULTI can properly catch the "Duplicate" message. Any ideas? I guess the reason is the warning is emitted first.

/Users/alex/Projects/solidity/test/libsolidity/SolidityNameAndTypeResolution.cpp:6595: error: in "SolidityNameAndTypeResolution/experimental_pragma": check err.type() == (Error::Type::SyntaxError) has failed
Expected message "Duplicate experimental feature name." but found "Experimental features are turned on. Do not use experimental features on live deployments.".
/Users/alex/Projects/solidity/test/libsolidity/SolidityNameAndTypeResolution.cpp:6595: error: in "SolidityNameAndTypeResolution/experimental_pragma": check searchErrorMessage(err, ("Duplicate experimental feature name.")) has failed

Alternatively we could allow duplicates.

@axic axic force-pushed the experimental-metadata branch 2 times, most recently from 95dc2a9 to 873aab2 Compare August 11, 2017 14:32
@axic axic force-pushed the experimental-metadata branch from 873aab2 to 2d1bab0 Compare August 11, 2017 15:39
@axic
Copy link
Contributor Author

axic commented Aug 11, 2017

@chriseth skipping that one test (to be fixed separately), should be ready

@axic axic merged commit e3d1137 into develop Aug 11, 2017
@axic axic removed the in progress label Aug 11, 2017
@axic axic deleted the experimental-metadata branch August 11, 2017 17:21
@axic axic mentioned this pull request Aug 14, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@chriseth chriseth chriseth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@axic @chriseth