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

Add support for { from: '' } param in call method #126

Merged
merged 10 commits into from Aug 16, 2018

Conversation

Projects
None yet
3 participants
@leftab
Copy link
Contributor

leftab commented Jul 14, 2018

This closes #125
With this change, a developer can specify the call's sender address like this:

app.call('foo', 'bar', { from: '0x5f6f7e8cc7346a11ca2def8f827b7a0b612c56a1' })
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Jul 14, 2018

CLA assistant check
All committers have signed the CLA.

@sohkai
Copy link
Member

sohkai left a comment

❤️ this! Thanks for adding this in, @max-cote!

return this.contract.methods[method](...params.slice(0,-1)).call({
...(from && { from })
})
}

This comment has been minimized.

@sohkai

sohkai Jul 16, 2018

Member

A few nitpicks on the above block:

  • lastParam doesn't seem to be defined
  • ...params.slice(0,-1) doesn't need the spread, since it makes a new array already
    • I'm not sure why it was being spread in the original case, and we can probably remove that
  • We can probably just pass in the entire lastParam to call() so any of the optional parameters are supported
  • Would be great to document here: https://github.com/aragon/aragon.js/blob/master/docs/APP.md#call
})
}
else {
return this.contract.methods[method](...params).call()

This comment has been minimized.

@sohkai

sohkai Jul 16, 2018

Member

This bit doesn't need the else, since the above block will return if executed.

sohkai added some commits Jul 17, 2018

Wrapper: upgrade dependencies (#127)
* Wrapper: upgrade apm.js to 2.0.0

* Wrapper: upgrade radspec to 1.0.0-beta.8

* Wrapper: remove unnecessary ipfs-api dependency

* Wrapper: update package-lock.json
@leftab

This comment has been minimized.

Copy link
Contributor

leftab commented Jul 30, 2018

Thanks a lot for the feedback @sohkai ! I should be able to update the code in a couple of days.

@leftab

This comment has been minimized.

Copy link
Contributor

leftab commented Aug 13, 2018

Ok sorry for the hude delay! I guess we were all a little bit too optimistic about what we would be able to do in the first week of our project! haha

So thanks again for this thoughtful feedback! Here is what I found:

  • You are totally right! 😄 My bad, too much refactoring. This should be fixed.
  • After playing a little bit with web3, the spread is indeed necessary in some cases. e.g. calling a method that doesn't take any parameters with an empty array will result in the following error: Invalid number of parameters for "..." . Got 1 expected 0!
  • Agree! Done!
  • Also done :o)
  • And now that the lastParam is back, the else is useful again hehe
@sohkai

sohkai approved these changes Aug 14, 2018

Copy link
Member

sohkai left a comment

🎉 Nice @leftab!

Sorry about the comment on spreading and if it misled you; mistook the web3.js API as taking in an array rather than an argument list :).

Looking really good now, just a few cosmetic comments!

docs/APP.md Outdated
@@ -230,6 +230,10 @@ Perform a read-only call on the app's smart contract.

1. `method` (`String`): The name of the method to call.
2. `...params` (*arguments*): An optional variadic number of parameters.
3. `options` (`Object`): Call options (Optional).

This comment has been minimized.

@sohkai

sohkai Aug 14, 2018

Member

I would just link to the options object documentation on web3.js' side.

This comment has been minimized.

@leftab

leftab Aug 16, 2018

Contributor

Done 🙂

return this.contract.methods[method](...params.slice(0,-1)).call(lastParam)
}
else {
return this.contract.methods[method](...params).call()

This comment has been minimized.

@sohkai

sohkai Aug 14, 2018

Member

This else isn't necessary, since if the previous condition was true, it'd never reach here :).

What do you think about making this a ternary? I like it because the alignment makes it basically a diff of the two options.

return (typeof lastParam === 'object' && lastParam !== null)
  ? this.contract.methods[method](...params.slice(0,-1)).call(lastParam)
  : this.contract.methods[method](...params).call()

This comment has been minimized.

@leftab

leftab Aug 16, 2018

Contributor

Oh sorry I didn't understand what you meant the other day. Yeah I have to say it is cleaner that way! :)

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Aug 16, 2018

@leftab Thanks again 🙏!

@sohkai sohkai merged commit 48d414f into aragon:master Aug 16, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment