search

autonomoussoftware / metronome

Metronome autonomous system
MIT License
14 stars 10 forks source link

Removed duplicate code from multiSubwithdraw #53

Closed rokso closed 6 years ago

rokso commented 6 years ago

Fixed number 10 and 14 from issue #40

  1. Use address array instead of uint array
  2. Changed subwithdrawFor to use if condition instead of require checks
  3. Removed duplicate code from multiSubwithdrawFor
  4. Updated test according to removal of require check
  5. Update timed-test to use shared time travel util
rokso commented 6 years ago

@filcap @patidarmanoj10 Please review the changes.

patidarmanoj10 commented 6 years ago

LGTM

filcap commented 6 years ago

Since we are changing it so the internal subwithdrawFor call does not throw for inactive subscriptions, should we consider making function subWithdraw(address _from) public transferable returns (bool) throw if the internal call returns false?

Other than that, the code changes look good.

@rokso @patidarmanoj10

patidarmanoj10 commented 6 years ago

@rokso I agreed. We should throw because caller will confuse when call does not revert. will be misleading that withdraw does not happen and function not reverted also. This is for single subwithdraw function

rokso commented 6 years ago

@filcap @patidarmanoj10 right now multiSubwithdraw is using the subwithdraw function and failing subwithdraw will fail multiSubwithdraw. I think we can directly call subwithdrawFor from multiSubwithdraw and allow subwithdraw to have a require check. This will solve the both issue.

rokso commented 6 years ago

@patidarmanoj10 @filcap I have fixed it as suggested. I have also tried to fix proceeds.js test which happens to fail on Travis from time to time.