Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Address Book Suggestions #1064

Merged
merged 13 commits into from
Jul 3, 2020
Merged

Conversation

matextrem
Copy link
Contributor

closes #916

@matextrem matextrem added Enhancement ✨ Minor Improvement / changes to existing functionality Major Needs to be fixed for immediate next public release. labels Jun 26, 2020
@matextrem matextrem added this to the Sprint 9 milestone Jun 26, 2020
@matextrem matextrem self-assigned this Jun 26, 2020
@gnosis-info
Copy link

Travis automatic deployment:
https://pr1064--safereact.review.gnosisdev.com/app

@lukasschor
Copy link
Member

image

Not really related to this PR, but this input validation for the Custom Transaction is wrong, the value can indeed be 0. Maybe we can fix this in this PR, but if it takes more than half an hour or so, I would suggest creating a new ticket and prioritizing it in the backlog.

@lukasschor
Copy link
Member

Other than that it looks good to me. :)

@matextrem
Copy link
Contributor Author

matextrem commented Jun 26, 2020

@lukasschor The thing is that you can just leave empty the field if you don’t want to add a value. That’s the reason why I thought if you add a value it is because is greater than 0. Otherwise you can leave it empty. Anyway, I can change the validator to support 0 as a valid value.

@lukasschor
Copy link
Member

Yeah I think we should change it (if it's little work), as it's not clear that the field can be empty (we even show a * which usually means it's a mandatory field).

@gnosis-info
Copy link

Travis automatic deployment:
https://pr1064--safereact.review.gnosisdev.com/app

Copy link
Contributor

@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.

I left a couple of observations. I don't think they're mandatory. But if there's time to make the change, I prefer we go with that approach.

@matextrem matextrem requested a review from mmv08 June 29, 2020 16:50
@francovenica
Copy link
Contributor

@matextrem @lukasschor Questions.

1-You can select not only contracts that have an ABI but also other types of contracts like other safes or tokens contracts. I'm assuming that the system has no way to tell that they are different (all contracts I suppose) right?

2-In the Contract interaction form for smart contracts, if you pick an address and the try to "re pick" another it wont let you, it won't show the address book suggestions again. This does not happen for the "custom Tx" form, where even if you pic an address you can double click in the field and the address book suggestions will popUp again.
I don't remember if it always worked like that, but can we make the Smart contract form to suggest another addresses even if you already picked an address?
06-29-2020_x(716).gif

@lukasschor
Copy link
Member

  1. is fine, users might want to interact with safe contracts or token contracts as well.

  2. yeah that would be nice

@francovenica
Copy link
Contributor

@matextrem It would be possible to do the number 2?

Now if you think it takes a long time then I can create a new ticket and close this one.

@matextrem
Copy link
Contributor Author

@francovenica Yup, I think we could create another ticker and assign it to me since that thing is not related to this ticket itself and it always worked this way.

@francovenica
Copy link
Contributor

Created the #1080 for point 2 then.

@francovenica francovenica self-requested a review June 29, 2020 20:14
@gnosis-info
Copy link

Travis automatic deployment:
https://pr1064--safereact.review.gnosisdev.com/app

@matextrem
Copy link
Contributor Author

@mikheevm could you please approve this PR?

@gnosis-info
Copy link

Travis automatic deployment:
https://pr1064--safereact.review.gnosisdev.com/app

@gnosis-info
Copy link

Travis automatic deployment:
https://pr1064--safereact.review.gnosisdev.com/app

@gnosis-info
Copy link

Travis automatic deployment:
https://pr1064--safereact.review.gnosisdev.com/app

@matextrem matextrem merged commit 3fdac53 into development Jul 3, 2020
@matextrem matextrem deleted the feature/address-book-suggestions branch July 3, 2020 19:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Enhancement ✨ Minor Improvement / changes to existing functionality Major Needs to be fixed for immediate next public release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Address Book Suggestions for Smart Contract Interactions
6 participants