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.

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

(Feature) Import contract #212

Merged
merged 58 commits into from
Dec 11, 2018
Merged

(Feature) Import contract #212

merged 58 commits into from
Dec 11, 2018

Conversation

vbaranov
Copy link
Collaborator

@vbaranov vbaranov commented Dec 4, 2018

Import a new type of account: smart-contract with the ability to execute transactions on it and call the data from it.

This type of account (address and ABI) is saved in the keyrings as a usual account.

A life cycle of reading the data from the contract:

  • choose saved "contract" account
  • click 'send' on the main screen
  • in the list of contract methods, choose the method to call
  • fill the appeared input fields and click 'Call data'
  • the result will be in 'Output' field

A life cycle of executing the data from the contract:

  • choose saved "contract" account
  • click 'send' on the main screen
  • in the list of contract methods, choose the method to execute
  • fill the appeared input fields and click 'Next'
  • in the appeared screen with the list of unlocked accounts choose the account to execute transaction on the contract and click 'Next'
  • confirm transaction (as usual) to contract. It will be executed under account selected in the list on the previous step.

Screenshots of the feature:

Import contract:

screen shot 2018-12-04 at 18 31 45

Execute transaction:

screen shot 2018-12-04 at 18 33 13

image

Call contract's data:

screen shot 2018-12-04 at 18 39 42

Build on top of #209 (should merged after it)

@vbaranov
Copy link
Collaborator Author

vbaranov commented Dec 7, 2018

The ability to copy ABI encoded function call has been added 5ccda3c

screen shot 2018-12-07 at 20 05 17

@dennis00010011b
Copy link

@vbaranov
Execute Method: there is no validation of parameter if click button 'Next' , but parameters still validated by clicking button 'Copy ABI encoded'

tests for the button 'Copy ABI encoded'
@fvictorio
Copy link

@vbaranov I deployed an ERC20 contract in sokol (address 0x3d71971291CeD21D88Cb1f6f121C915871B6CF5c) but when I import it in Nifty Wallet it is added like a regular account.

I went to "Import account", changed the select box to "Contract", added that address and pasted the ERC20 ABI.

@vbaranov
Copy link
Collaborator Author

@fvictorio
Repeated the same for the mentioned account - works for me.
screen shot 2018-12-11 at 22 51 28

Could you specify, does it have a Contract label? Does the screen with methods of contract open when you click to 'Send' button? What are the steps to reproduce this issue?

@vbaranov
Copy link
Collaborator Author

@dennis00010011b

Execute Method: there is no validation of parameter if click button 'Next' , but parameters still validated by clicking button 'Copy ABI encoded'

Thanks, actually, validation was ignored for 'Next' button. Now, I check the status of method ABI encoding. Fixed here 715668a

@fvictorio
Copy link

Does the screen with methods of contract open when you click to 'Send' button?

My bad, that was the step I was missing.

Copy link

@fernandomg fernandomg left a comment

Choose a reason for hiding this comment

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

LGTM, the only thing that feels a bit awkward when you send a tx and reject it. The account that remains selected is the account used to sign the tx and not the contract.

But I think that can be solved in a different issue.

Copy link

@patitonar patitonar left a comment

Choose a reason for hiding this comment

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

Here are some comments about UX for future improvements:

  • Adding a contract is difficult. You have to copy and paste the address and the ABI. After pasting one of them, you lose focus on the app to copy the other one and it closes. So you have to start over and have the same problem again. May be an option is to allow the user to open the app on the browser tab, as Metamask does.
  • When you select a contract, it looks the same as an account, here are some ideas:
  1. Instead of saving the contract as "Account X", save it as default name of "Contract X"
  2. Instead of displaying Buy and Send buttons we can display something more explicit like a execute methods button.
  3. An other option could be to display Execute Method view directly on the main page, instead of Sent/Tokens tabs we could replace Tokens with this view.

@vbaranov
Copy link
Collaborator Author

@fernandomg @patitonar thank you for the valuable suggestions. I keep them in mind for 4.10+ releases. All of them are conjugated with some sort of refactoring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants