Skip to content
This repository was archived by the owner on Oct 28, 2021. It is now read-only.

JSON accounts: allow loading code from a file #4783

Merged
merged 3 commits into from
Jan 24, 2018
Merged

Conversation

chfast
Copy link
Member

@chfast chfast commented Jan 17, 2018

Fixes #4774

@chfast chfast requested review from axic and winsvega January 17, 2018 18:44
};
set<string> const c_knownParamNames = {c_minGasLimit, c_maxGasLimit, c_gasLimitBoundDivisor,
c_homesteadForkBlock, c_EIP150ForkBlock, c_EIP158ForkBlock, c_accountStartNonce,
c_maximumExtraDataSize, c_tieBreakingGas, c_blockReward, c_byzantiumForkBlock, c_eWASMForkBlock,
Copy link
Member Author

Choose a reason for hiding this comment

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

Here c_eWASMForkBlock is added. I missed it previously.

Copy link
Member

Choose a reason for hiding this comment

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

Do you want this into another pull request and merge it now?

string const c_storage = "storage";
string const c_shouldnotexist = "shouldnotexist";
string const c_precompiled = "precompiled";
std::set<string> const c_knownAccountFields = {
c_wei, c_finney, c_balance, c_nonce, c_code, c_storage, c_shouldnotexist,
c_wei, c_finney, c_balance, c_nonce, c_code, c_file, c_storage, c_shouldnotexist,
Copy link
Contributor

Choose a reason for hiding this comment

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

you put path to the file with code as an attribute of account object?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the idea was to load code from file upon loading genesis.json
why would we need to change logic of Accont.h ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Load a code from a file what do what with it?

Copy link
Contributor

Choose a reason for hiding this comment

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

so as I understand currently account's code is loaded from genesis.
could we change it so that upon loading that genesis account code will just be read from external file? other then that it is the same genesis config file loading.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand

Copy link
Contributor

@winsvega winsvega Jan 18, 2018

Choose a reason for hiding this comment

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

in jsonToAccountMap at

if (o[c_code].type() == json_spirit::str_type)
{
   if (o[c_code].get_str().substr(0,2) == "//")
        read code from file in this link and parse it.
}

I thought we could just do it that way.
so in genesis config file at accounts code it will be

"code": "//path/to/file"

Copy link
Member

Choose a reason for hiding this comment

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

I think it is better making it more explicit than hiding it via special expressions.

@@ -88,11 +90,12 @@ namespace
string const c_balance = "balance";
string const c_nonce = "nonce";
string const c_code = "code";
string const c_file = "file"; ///< A file containg a code as bytes.
Copy link
Member

Choose a reason for hiding this comment

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

I think file may be misleading to mean loading an account JSON from a file, e.g{accounts: { "0x0044...": { file: "/path" } } }

Copy link
Member Author

Choose a reason for hiding this comment

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

Back to "codeInFile" or "codeFromFile"?

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind, probably the second is more readable.

@codecov-io
Copy link

codecov-io commented Jan 18, 2018

Codecov Report

Merging #4783 into develop will decrease coverage by <.01%.
The diff coverage is 41.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #4783      +/-   ##
==========================================
- Coverage     60.7%   60.7%   -0.01%     
==========================================
  Files          348     348              
  Lines        27384   27405      +21     
  Branches      2845    2850       +5     
==========================================
+ Hits         16624   16636      +12     
- Misses        9761    9780      +19     
+ Partials       999     989      -10

if (codeObj.type() == json_spirit::str_type)
{
auto& codeStr = codeObj.get_str();
if (codeStr.find("0x") != 0 && !codeStr.empty())
Copy link
Contributor

Choose a reason for hiding this comment

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

codeStr.find("0x") could produce an error.
0x should be at located at first 2 bytes of the code field.

Copy link
Member

Choose a reason for hiding this comment

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

If you look at the diff, this code is as is before. No changes.

Copy link
Contributor

@winsvega winsvega Jan 20, 2018

Choose a reason for hiding this comment

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

I know. I just saw an issue.
11220x3345
will pass.

Copy link
Member

Choose a reason for hiding this comment

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

I think that could be addressed in a separate PR then?

@chfast
Copy link
Member Author

chfast commented Jan 22, 2018

Ready?

@axic
Copy link
Member

axic commented Jan 22, 2018

The field name wasn't changed from file.

@chfast
Copy link
Member Author

chfast commented Jan 22, 2018

Changed and rebased. One commit less.

@axic
Copy link
Member

axic commented Jan 23, 2018

@pirapira okay to merge?

@pirapira
Copy link
Member

I couldn't spot the offending failure in AppVeyor with a quick look. Coming back later.

@axic
Copy link
Member

axic commented Jan 24, 2018

Test Case "bcRandomBlockhashTest": 
.
randomStatetest165BC_EIP158Error importing block from fields to blockchain: C:\projects\cpp-ethereum\libdevcore\TransientDirectory.cpp(41): Throw in function __cdecl dev::TransientDirectory::TransientDirectory(const class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > &)
Dynamic exception type: class boost::exception_detail::clone_impl<struct dev::FileError>
std::exception::what: FileError
C:/projects/cpp-ethereum/test/tools/jsontests/BlockChainTests.cpp(566): error: in "BlockchainTests/bcRandomBlockhashTest": randomStatetest165BC_EIP158State root in chain from RLP blocks != State root in chain from Field blocks!
C:/projects/cpp-ethereum/test/tools/libtesteth/ImportTest.cpp(496): error: in "BlockchainTests/bcRandomBlockhashTest": randomStatetest165BC_EIP158 Check State: a94f5374fce5edbc8e2a8697c15331677e6ebf0b: incorrect balance 1000000000000000000, expected 999999999999858890

Can you restart Appveyor? It is in the "random" tests, perhaps the failure is actually random :)

@pirapira
Copy link
Member

I cannot restart it.

@chfast
Copy link
Member Author

chfast commented Jan 24, 2018

Restarted.

@winsvega
Copy link
Contributor

this is probably the issue with blockchain tmp files collision.

@axic
Copy link
Member

axic commented Jan 24, 2018

@pirapira appveyor suceeded 🍾

@pirapira pirapira merged commit 2716b63 into develop Jan 24, 2018
@pirapira pirapira deleted the json-accounts branch January 24, 2018 14:33
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants