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.

Sign up for GitHub

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

Jump to bottom

[isoltest] Add support to query balance. #10873

Merged
merged 1 commit into from
Apr 28, 2021
Merged

[isoltest] Add support to query balance. #10873

merged 1 commit into from
Apr 28, 2021

Conversation

aarlt
Copy link
Member

@aarlt aarlt commented Feb 1, 2021 •
edited by axic
Loading

Depends on #10867
Fixes #10426.

@aarlt aarlt force-pushed the isoltest-builtins branch from c180cc3 to 7903410 Compare February 1, 2021 20:04
@aarlt aarlt marked this pull request as draft February 1, 2021 20:20
@aarlt aarlt force-pushed the isoltest-builtins branch 4 times, most recently from 18a3cfe to d4dc5c3 Compare February 15, 2021 11:58
@roru78
Copy link

roru78 commented Feb 15, 2021

Нет описания.

isoltest-balance

@aarlt aarlt force-pushed the isoltest-builtins branch 11 times, most recently from afc5e06 to df5ae1a Compare February 16, 2021 18:34
@aarlt aarlt force-pushed the isoltest-builtins branch from df5ae1a to 3dbdea9 Compare February 16, 2021 19:49
@aarlt aarlt force-pushed the isoltest-balance branch 3 times, most recently from a8430c5 to 30a3eeb Compare February 16, 2021 20:16
@aarlt aarlt marked this pull request as ready for review February 16, 2021 20:28
@aarlt aarlt force-pushed the isoltest-builtins branch 2 times, most recently from 22d9e9b to 71d1a9e Compare February 17, 2021 16:36
@leonardoalt
Copy link
Member

Needs rebase

@leonardoalt
Copy link
Member

I rebased this, hope I didn't screw anything up, there was a one line conflict in SemanticTest.h.
Other than that I think it's good to go?

@chriseth
Copy link
Contributor

Something is not right.

@aarlt aarlt force-pushed the isoltest-balance branch from 66ef23b to 991fb6c Compare April 23, 2021 05:03
aarlt
aarlt commented Apr 23, 2021
View reviewed changes
test/libsolidity/util/TestFileParser.cpp Outdated
@@ -127,6 +127,8 @@ vector<solidity::frontend::test::FunctionCall> TestFileParser::parseFunctionCall
tie(call.signature, lowLevelCall) = parseFunctionSignature();
if (lowLevelCall)
call.kind = FunctionCall::Kind::LowLevel;
if (isBuiltinFunction(call.signature))
Copy link
Member Author

@aarlt aarlt Apr 23, 2021 •
edited
Loading

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.

Copy link
Contributor

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?

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, makes sense. Just created #11304.

@aarlt aarlt force-pushed the isoltest-balance branch 2 times, most recently from bb58b29 to f8d8802 Compare April 26, 2021 18:46
axic
axic reviewed Apr 26, 2021
View reviewed changes
test/libsolidity/semanticTests/builtins/balance_other_contract.sol Outdated
@@ -0,0 +1,29 @@
contract Other {
Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

chriseth
chriseth reviewed Apr 27, 2021
View reviewed changes
test/libsolidity/SemanticTest.cpp Outdated
@@ -131,6 +132,19 @@ void SemanticTest::initializeBuiltins()
{
return util::toBigEndian(u256(0x1234));
};
soltestAssert(m_builtins.count("accountBalance") == 0, "");
m_builtins["accountBalance"] = [this](FunctionCall const& _call) -> std::optional<bytes>
Copy link
Contributor

@chriseth chriseth Apr 27, 2021 •
edited
Loading

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?

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, thats possible.

@aarlt aarlt force-pushed the isoltest-balance branch 2 times, most recently from 0e97f53 to 1c674ab Compare April 27, 2021 22:45
@chriseth chriseth enabled auto-merge April 28, 2021 13:57
axic
axic reviewed Apr 28, 2021
View reviewed changes
test/libsolidity/semanticTests/isoltestTesting/balance_without_balance.sol
// compileViaYul: also
// ----
// balance -> 0
// balance: 0x0000000000000000000000000000000000000000 -> 0
Copy link
Member

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

no

@axic axic mentioned this pull request Apr 28, 2021
Closed
axic
axic reviewed Apr 28, 2021
View reviewed changes
test/libsolidity/semanticTests/smoke/constructor.sol
@@ -15,6 +15,6 @@ contract C {
// ----
// constructor(), 2 wei: 3 ->
// state() -> 3
// balance() -> 2
Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll use both.

axic
axic reviewed Apr 28, 2021
View reviewed changes
test/libsolidity/semanticTests/smoke/fallback.sol Outdated
// ----
// data() -> 0
// ()
// data() -> 1
// (): hex"42ef"
// data() -> 2
// externalData() -> 0x20, 2, left(0x42ef)
// balance() -> 0
Copy link
Member

@axic axic Apr 28, 2021 •
edited
Loading

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll revert.

Copy link
Contributor

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 chriseth disabled auto-merge April 28, 2021 14:05
chriseth
chriseth suggested changes Apr 28, 2021
View reviewed changes
Copy link
Contributor

@chriseth chriseth left a 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 ().

@chriseth
Copy link
Contributor

The above is implemented in #11326 Rebasing on top of that.

@chriseth chriseth changed the base branch from develop to builtin_user_function April 28, 2021 14:19
Base automatically changed from builtin_user_function to develop April 28, 2021 14:56
@chriseth chriseth merged commit 850c25b into develop Apr 28, 2021
@chriseth chriseth deleted the isoltest-balance branch April 28, 2021 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axic axic axic left review comments

@leonardoalt leonardoalt leonardoalt left review comments

@mijovic mijovic mijovic left review comments

@chriseth chriseth chriseth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[isoltest] Add balance: statement
6 participants
@aarlt @roru78 @axic @leonardoalt @chriseth @mijovic