-
Notifications
You must be signed in to change notification settings - Fork 6.3k
[isoltest] Add support to query balance. #10873
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
Conversation
c180cc3 to
7903410
Compare
7def49c to
2b55e88
Compare
test/libsolidity/semanticTests/builtins/balance_other_contract.sol
Outdated
Show resolved
Hide resolved
18a3cfe to
d4dc5c3
Compare
isoltest-balance |
afc5e06 to
df5ae1a
Compare
2b55e88 to
026bc82
Compare
df5ae1a to
3dbdea9
Compare
a8430c5 to
30a3eeb
Compare
22d9e9b to
71d1a9e
Compare
|
Needs rebase |
b5bc8dc to
66ef23b
Compare
|
I rebased this, hope I didn't screw anything up, there was a one line conflict in SemanticTest.h. |
|
Something is not right. |
| tie(call.signature, lowLevelCall) = parseFunctionSignature(); | ||
| if (lowLevelCall) | ||
| call.kind = FunctionCall::Kind::LowLevel; | ||
| if (isBuiltinFunction(call.signature)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mijovic Looks like that you accidentally removed these lines. Strange that we did not have any isoltest builtin function tests in place. We should merge this PR soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I am not sure how I did that... Does it makes sense to extract this out and add as separate PR to merge these back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, makes sense. Just created #11304.
bb58b29 to
f8d8802
Compare
| @@ -0,0 +1,29 @@ | |||
| contract Other { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rename this directory to isoltestTesting or something else, but builtins here suggests precompiles or some solidity builtins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
test/libsolidity/SemanticTest.cpp
Outdated
| return util::toBigEndian(u256(0x1234)); | ||
| }; | ||
| soltestAssert(m_builtins.count("accountBalance") == 0, ""); | ||
| m_builtins["accountBalance"] = [this](FunctionCall const& _call) -> std::optional<bytes> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we only use a single builtin called balance which has an optional parameter that defaults to the contract address?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thats possible.
0e97f53 to
1c674ab
Compare
1c674ab to
5c90b12
Compare
| // compileViaYul: also | ||
| // ---- | ||
| // balance -> 0 | ||
| // balance: 0x0000000000000000000000000000000000000000 -> 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the wei/ether suffix works on the returned value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
| // ---- | ||
| // constructor(), 2 wei: 3 -> | ||
| // state() -> 3 | ||
| // balance() -> 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should remove the balance() function from this contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll use both.
| // (): hex"42ef" | ||
| // data() -> 2 | ||
| // externalData() -> 0x20, 2, left(0x42ef) | ||
| // balance() -> 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should remove the balance function in this contract.
This contract seems to have balance() as a public variable. Are we sure this change here is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it seems you cannot call a function called balance anymore.
chriseth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The builtin mechanism has to be changed, it still has to be possible to call all functions. Builtins and user-defined functions can be distinguished by ().
|
The above is implemented in #11326 Rebasing on top of that. |
5c90b12 to
19ad9fa
Compare
Depends on #10867
Fixes #10426.