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

[WIP] Allow constants access through contract names #3214

Open
wants to merge 5 commits into
base: develop
from

Conversation

Projects
4 participants
@kseo
Contributor

kseo commented Nov 16, 2017

Fixes #1290.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Nov 16, 2017

Contributor

Thanks, that's a very good start!

Contributor

chriseth commented Nov 16, 2017

Thanks, that's a very good start!

@kseo

This comment has been minimized.

Show comment
Hide comment
@kseo

kseo Nov 16, 2017

Contributor

@chriseth What other places do I need to change besides TypeType::nativeMembers?

Contributor

kseo commented Nov 16, 2017

@chriseth What other places do I need to change besides TypeType::nativeMembers?

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Nov 16, 2017

Member

@kseo you could add test cases first and see what is not working and/or what was broken by the change and track it down from there

Member

axic commented Nov 16, 2017

@kseo you could add test cases first and see what is not working and/or what was broken by the change and track it down from there

@ekpyron ekpyron self-assigned this Apr 5, 2018

@axic

At the minimum this needs tests and changelog entry.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Apr 5, 2018

Member

Since constant can contain expressions (which are copied and evaluated a the place they are used) and refer to other constants it probably makes sense to have a complex test case, such as:

contract C {
  uint constant a = 33 * b + 4; // can also mix keccak256(b) into this
  uint constant b = 44;
}

contract D {
  uint constant b = 66;
  function f() returns (uint) {
    return C.a;
  }
}

The goal here is to ensure that b is looked up from the correct contract when evaluating the constant expression.

Member

axic commented Apr 5, 2018

Since constant can contain expressions (which are copied and evaluated a the place they are used) and refer to other constants it probably makes sense to have a complex test case, such as:

contract C {
  uint constant a = 33 * b + 4; // can also mix keccak256(b) into this
  uint constant b = 44;
}

contract D {
  uint constant b = 66;
  function f() returns (uint) {
    return C.a;
  }
}

The goal here is to ensure that b is looked up from the correct contract when evaluating the constant expression.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Apr 5, 2018

Member

This reminds me (it is not strictly related to this PR), but we could add tests for storage constant and memory constant.

Member

axic commented Apr 5, 2018

This reminds me (it is not strictly related to this PR), but we could add tests for storage constant and memory constant.

@ekpyron ekpyron changed the title from [WIP] Allow constants access through contract names (issue #1290) to Allow constants access through contract names (issue #1290) Apr 6, 2018

PR updated.

@axic axic requested a review from chriseth Apr 11, 2018

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Apr 11, 2018

Member

@chriseth this needs to be reviewed

Member

axic commented Apr 11, 2018

@chriseth this needs to be reviewed

@chriseth

Please add a test for

  • cross-contract cycle checks
compileAndRun(sourceCode, 0, "Test");
ABI_CHECK(callContractFunction("f()"), encodeArgs(u256(1456)));
}

This comment has been minimized.

@chriseth

chriseth Apr 11, 2018

Contributor

Are there any other weird cases we have to test? Is it somehow possible to use super or this in that context?

@chriseth

chriseth Apr 11, 2018

Contributor

Are there any other weird cases we have to test? Is it somehow possible to use super or this in that context?

This comment has been minimized.

@chriseth

chriseth Apr 11, 2018

Contributor

Is it possible to access constants via super? If yes, does that work correctly if that constant is accessed from another contract?

@chriseth

chriseth Apr 11, 2018

Contributor

Is it possible to access constants via super? If yes, does that work correctly if that constant is accessed from another contract?

This comment has been minimized.

@ekpyron

ekpyron Apr 13, 2018

Contributor

Access via super is not allowed so far - I think it'd be better to have to use the name of the base explicitly instead. I can change that, though, if you think otherwise. In any case I added a test (for now with the expectation that access through super is not possible).

@ekpyron

ekpyron Apr 13, 2018

Contributor

Access via super is not allowed so far - I think it'd be better to have to use the name of the base explicitly instead. I can change that, though, if you think otherwise. In any case I added a test (for now with the expectation that access through super is not possible).

@ekpyron ekpyron changed the title from Allow constants access through contract names (issue #1290) to [WIP] Allow constants access through contract names (issue #1290) Apr 12, 2018

@ekpyron

This comment has been minimized.

Show comment
Hide comment
@ekpyron

ekpyron Apr 12, 2018

Contributor

@chriseth I added some more test cases, but indeed cross-contract cycle detection does not currently work. Since this PR was not originally planned for 0.4.22, I suggest that I first continue to review the other PRs and then get back to this, unless you definitely want this in the current release.

Contributor

ekpyron commented Apr 12, 2018

@chriseth I added some more test cases, but indeed cross-contract cycle detection does not currently work. Since this PR was not originally planned for 0.4.22, I suggest that I first continue to review the other PRs and then get back to this, unless you definitely want this in the current release.

@chriseth chriseth added this to To do in 0.5.0 via automation Apr 12, 2018

@ekpyron ekpyron changed the title from [WIP] Allow constants access through contract names (issue #1290) to Allow constants access through contract names (issue #1290) Apr 12, 2018

@ekpyron

This comment has been minimized.

Show comment
Hide comment
@ekpyron

ekpyron Apr 13, 2018

Contributor

Cross contract cycle detection should work now and this is ready for review.

Contributor

ekpyron commented Apr 13, 2018

Cross contract cycle detection should work now and this is ready for review.

PR updated.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Apr 13, 2018

Contributor

Please add a test case for

contract A {
    function f() pure returns (uint) { return 1; }
}
contract B is A {
    uint constant x = super.f();
}
contract Y {
    function f() pure returns (uint) { return 2; }
    
}
contract X is Y {
    uint constant a = B.x;
}

and check what the value of a is.

Contributor

chriseth commented Apr 13, 2018

Please add a test case for

contract A {
    function f() pure returns (uint) { return 1; }
}
contract B is A {
    uint constant x = super.f();
}
contract Y {
    function f() pure returns (uint) { return 2; }
    
}
contract X is Y {
    uint constant a = B.x;
}

and check what the value of a is.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Apr 13, 2018

Contributor

Note that we currently still allow non-compile-time-constant constants - we only add a warning. Because of that I would say this is too dangerous to include in the release.

Contributor

chriseth commented Apr 13, 2018

Note that we currently still allow non-compile-time-constant constants - we only add a warning. Because of that I would say this is too dangerous to include in the release.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Apr 23, 2018

Contributor

Did we test accessing address x = address(this) from different contracts?

Contributor

chriseth commented Apr 23, 2018

Did we test accessing address x = address(this) from different contracts?

@ekpyron ekpyron changed the title from Allow constants access through contract names (issue #1290) to [WIP] Allow constants access through contract names (issue #1290) May 2, 2018

@ekpyron

This comment has been minimized.

Show comment
Hide comment
@ekpyron

ekpyron May 2, 2018

Contributor

I updated the PR to include @chriseth's test (before the latest changes the test contract would cause a fatal compiler error; now the expected value of a is 1).

However:

  • In the meantime I agree that this PR should be postponed as long as non-compile-time-constant constants are possible.
  • One might actually argue that in the new cross_contract_constants_super_call test the value of a should be 2 and not 1, so this has to be discussed. In general I think it may be a good idea to postpone this PR until e.g. #3157 is settled.
Contributor

ekpyron commented May 2, 2018

I updated the PR to include @chriseth's test (before the latest changes the test contract would cause a fatal compiler error; now the expected value of a is 1).

However:

  • In the meantime I agree that this PR should be postponed as long as non-compile-time-constant constants are possible.
  • One might actually argue that in the new cross_contract_constants_super_call test the value of a should be 2 and not 1, so this has to be discussed. In general I think it may be a good idea to postpone this PR until e.g. #3157 is settled.
}
contract X is Y {
uint constant a = B.x;
function g() pure returns (uint) { return a; }

This comment has been minimized.

@axic

axic May 4, 2018

Member

@ekpyron why would you argue it would be reasonable to have this return 2?

@axic

axic May 4, 2018

Member

@ekpyron why would you argue it would be reasonable to have this return 2?

This comment has been minimized.

@ekpyron

ekpyron May 4, 2018

Contributor

@axic I would definitely expect 1 here, but then I might also expect

BOOST_AUTO_TEST_CASE(super)

to either result in 11 or 13 and not in 15... in that test super traverses the entire inheritance graph of the context from which it is first invoked - if I think about it that way, than I might expect it to prefer X's context, even if it occurs in B...

I would still want the result here to be 1, but I thought it might be good to discuss this wrt consistency issues anyways.

@ekpyron

ekpyron May 4, 2018

Contributor

@axic I would definitely expect 1 here, but then I might also expect

BOOST_AUTO_TEST_CASE(super)

to either result in 11 or 13 and not in 15... in that test super traverses the entire inheritance graph of the context from which it is first invoked - if I think about it that way, than I might expect it to prefer X's context, even if it occurs in B...

I would still want the result here to be 1, but I thought it might be good to discuss this wrt consistency issues anyways.

@ekpyron ekpyron changed the title from [WIP] Allow constants access through contract names (issue #1290) to [WIP] Allow constants access through contract names May 9, 2018

@chriseth chriseth added this to Planned in 0.5.1 via automation May 14, 2018

@chriseth chriseth removed this from To do in 0.5.0 May 14, 2018

ekpyron added some commits May 2, 2018

Changes ContractCompiler::superContract into superContractRange, whic…
…h also finds super contracts in the case of cross contract lookup.
@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jul 25, 2018

Member

What is the status of this? I am kind of lost.

Member

axic commented Jul 25, 2018

What is the status of this? I am kind of lost.

@ekpyron

This comment has been minimized.

Show comment
Hide comment
@ekpyron

ekpyron Aug 1, 2018

Contributor

@axic The current status of this is "postponed". We could think about re-evaluating this now, though.

Contributor

ekpyron commented Aug 1, 2018

@axic The current status of this is "postponed". We could think about re-evaluating this now, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment