Solidity: Refactoring: split `bool TypeChecker::visit(FunctionCall const& _functionCall)` into multiple functions

[chriseth]

[Meshugah]

Is this something I could work on? If so, could you point me in the right direction?

[chriseth]

Yes! Try to split it into logical chunks and come up with god names for the functions :)
Perhaps start with a very very simple change first so we have room for discussion.

[Meshugah]

@chriseth should i be assigned to this? Sorry, just asking for procedure, and will do!

[chriseth]

@Meshugah sorry, github does not allow assigning issues to people who have not yet contributed. It is noted here, please feel free to start!

[vs77bb]

Hi @Meshugah we just added a bounty on Gitcoin. Please click 'Start Work' and you'll be eligible to receive it upon completion of the work here 🙂

[Meshugah]

@vs77bb Where do I click Start Work? Could you put up a link or the steps to follow?

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

__This issue now has a funding of 200.0 DAI (200.0 USD @ $1.0/DAI) attached to it.__

• If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
  
  
  
  • Want to chip in? Add your own contribution here.
  
  
  • Questions? Checkout Gitcoin Help or the Gitcoin Slack
  
  
  • $26,983.43 more funded OSS Work available on the Gitcoin Issue Explorer
  
  

[vs77bb]

@Meshugah ^See above!

[gitcoinbot]

Issue Status: 1. Open 2. Cancelled

---

__The funding of 200.0 DAI (200.0 USD @ $1.0/DAI) attached to this issue has been cancelled by the bounty submitter__

• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $27,421.91 more funded OSS Work available on the Gitcoin Issue Explorer

[liangdzou]

I think this function can be separated based on the FunctionCall types.

[liangdzou]

more complexed than that...

[liangdzou]

should be separated based on the health condition/type requirement rules of a function call.
The hard part is to summarize the rules from the code.

[liangdzou]

Current order:

1. calculate isPure and argument type from arguments (update annotation for positional call)
2. call expression type checker
3. calculate function call kind (type conversion, struct construction, or function call)
4. check for type conversion (returned in this branch)
5. calculate isPure and membersRemovedForStructConstructor.
6. calculate function type and do some related check
7. check padding for literal
8. argument checker
9. return type checker

I think we can put the isPure calculation together after function call kind calculation and before type conversion check. Other functionalities should be separated easily.
@chriseth What do you think?

[liangdzou]

I think the calculation for isPure can stay in the main function. I will do a draft version first.

[liangdzou]

Since someone is working on it, I will work on another issue.
@Meshugah There are some ideas that might be useful for you.
I think the steps 3, 4, 6, 7, 8 are large enough for functions.

[Meshugah]

@liangdzou Appreciate the guidance!

[liangdzou]

no problem :-)

[Meshugah]

@liangdzou what test's could I run to see if the changes are running correctly/ where do i look for what I can pass into the function TypeChecker?

[chriseth]

@Meshugah just run isoltest.sh, it should cover pretty much everything. Please submit a pull request - it is advisable to even do that without running the tests in this case.

[liangdzou]

I usually run "build/test/tools/isoltest". I think it is somehow the same as @chriseth said. Sometimes, I also run solidity/test/cmdlineTests.sh. When you submit a PR, there will be an online test. You will find out what tests are important for you.

[gitcoinbot]

@Meshugah Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] warning (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[Meshugah]

I'm still working on this

[gitcoinbot]

@Meshugah Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] warning (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[Meshugah]

I'm still working on this, however if this needs to be passed on to someone else for it to be out faster that's understandable as well.

[gitcoinbot]

@Meshugah Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] warning (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

@Meshugah due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

• [x] warning (3 days)
• [x] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[Meshugah]

@chriseth I'd like to hand this off to someone else due to work constraints.

[chriseth]

OK, no worries! Thanks for your efforts!

[vs77bb]

@liangdzou Would you be interested in picking this one up? 🙂

[liangdzou]

Sure

[vs77bb]

@liangdzou Please pick it up on Gitcoin by clicking 'Work Started'!

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

__Work has been started__.

These users each claimed they can complete the work by 8 months, 3 weeks from now.
Please review their action plans below:

1) svenski123 has been approved to start work.

Hi - I'd like to pick this up if it's still available.

General approach would be to separate out logic that visits/accepts the FunctionCall child nodes and sets the annotation members (isPure, kind, type) from logic that just does syntax checking / error message generation.

There is also a clear split between type conversions and struct constructor/function calls so that is a good separation point as well.

Learn more on the Gitcoin Issue Details page.

2) liangdzou has applied to start work _(Funders only: approve worker | reject worker)_.

Current order:

1. calculate isPure and argument type from arguments (update annotation for positional call)
2. call expression type checker
3. calculate function call kind (type conversion, struct construction, or function call)
4. check for type conversion (returned in this branch)
5. calculate isPure and membersRemovedForStructConstructor.
6. calculate function type and do some related check
7. check padding for literal
8. argument checker
9. return type checker

I think we can put the isPure calculation together after function call kind calculation and before type conversion check. Other functionalities should be separated easily.

Learn more on the Gitcoin Issue Details page.

[liangdzou]

If @svenski123 is interested, I would suggest him to work on this one :-)

[svenski123]

@chriseth Could you approve either @lingdzou or myself to get started on this?

[chriseth]

@vs77bb could you please approve @svenski123 ? Thanks!

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

__Work for 200.0 DAI (200.0 USD @ $1.0/DAI) has been submitted by__:

1. @svenski123

@vs77bb please take a look at the submitted work:

• PR by @svenski123

---

• Learn more on the Gitcoin Issue Details page
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $49,621.01 more funded OSS Work available on the Gitcoin Issue Explorer

[svenski123]

I've submitted pull request #5275, please kindly review.

I've tried to separate AST descent and annotation logic from the checking/message generation logic, as the latter is a bit kitchen sink (i.e. the deprecated function call checks (sha3/keccak256), the VM implementation dependent checks (staticcall).

The ABIDecode logic could also be factored a bit but it was already in a separate method and thus out of scope.

[svenski123]

@vs77bb PR #5275 has been merged into develop - can this bounty be paid out?

[chriseth]

Implemented in #5275

[frankchen07]

hey @svenski123 - @vs77bb is currently out on vacation. I'll follow up with him when he gets back to ensure you're paid out. :)

[svenski123]

@frankchen07 Thanks for following up on this. BTW when will @vs77bb be back?

[frankchen07]

@svenski123 - he just got back yesterday. I'll ping him on Slack directly to make sure this happens, hopefully before Thanksgiving tomorrow. Thanks for following up!

[svenski123]

Gents - any chance of getting this bounty paid out before 2019?
Also, a Merry Christmas / Happy New Year to all of you!
@vs77bb @frankchen07 @chriseth

[gitcoinbot]

⚡️ A *Gitter Of Coins* Kudos has been sent to @svenski123 for this issue from @vs77bb. ⚡️ Nice work @svenski123! Your Kudos has automatically been sent in the ETH address we have on file.

[vs77bb]

@svenski123 Just paid + a kudos for you 🙂sincere apologies for the delay!

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

__The funding of 200.0 DAI (200.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @svenski123.__

• Learn more on the Gitcoin Issue Details page
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $44,347.55 more funded OSS Work available on the Gitcoin Issue Explorer