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

WA-238 Implementation Multisig TXs#31

Merged
apanizo merged 14 commits intofeature/WA-238-withdraw-ethfrom
feature/WA-238-implementation-multisig-tx
May 31, 2018
Merged

WA-238 Implementation Multisig TXs#31
apanizo merged 14 commits intofeature/WA-238-withdraw-ethfrom
feature/WA-238-implementation-multisig-tx

Conversation

@apanizo
Copy link
Contributor

@apanizo apanizo commented May 27, 2018

Description
In this PR I have introduced the logic for executing pending TXs once the threshold is reached. In other words, the functionality at UI level (remember I am using a very own mocked service) is "done".

@germartinez also the bug you reported related impossible usage of the safe for different owners is solved

@tschubotz Feel free to try it and let me know if you find any bug

See:
screenshot 2018-05-27 14 12 50

  • For TX which they are waiting to be confirmed the blue button of "PROCESS TRANSACTION" appears.
  • Once you click on it the UI is updated.

TODO

  • Check creation of safe and dailyLimit module follow same steps as tests
  • DOM tests simulating user implementing Multisig TX
  • Modify the mocked backend service to fulfill requirements of SAFE SERVICES doc
  • Remove google font and load it locally
  • Check tx status '0x00 0x01' in testnets exception is nor raised
    https://github.com/ethereum/wiki/wiki/JavaScript-API#web3ethgettransactionreceipt
  • Storybook components for new components and routes

@apanizo apanizo requested review from germartinez and tschubotz May 27, 2018 12:22
@apanizo
Copy link
Contributor Author

apanizo commented May 27, 2018

Travis automatic deployment:
https://staging-31-pr-157-safe-react-gnosis.surge.sh

Storybook book automatic deployment:
https://storybook-staging-31-pr-157-safe-react-gnosis.surge.sh

@apanizo
Copy link
Contributor Author

apanizo commented May 28, 2018

Travis automatic deployment:
https://staging-31-pr-34-safe-react-gnosis.surge.sh

Storybook book automatic deployment:
https://storybook-staging-31-pr-34-safe-react-gnosis.surge.sh

Copy link
Contributor

@tschubotz tschubotz left a comment

Choose a reason for hiding this comment

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

@apanizo just played around a bit with it. Here's what I found.

(1) Minor validation Bug:

  • During Safe creation, enter 1.45346457 for number of confirmations
  • Expected: Validation error saying, that I need to enter an integer.
  • Observed: No error, it even is show on the Safe detail page
    image

(2) Question:
Why do I need to add a transaction name? Is this like a description field when making a bank transfer or what was your intention behind this?
image

(3) Issue with confirmations

  • I have created a Safe with 2 owners and confirmation threshold 1
  • I create a transaction and sign it
  • The transaction is not executed but waits for the 2nd confirmation which should not be necessary, right?
    image
  • It also shows "2 confirmations needed" on the screen
    image

@apanizo
Copy link
Contributor Author

apanizo commented May 29, 2018

@tschubotz Question 1 and Question 3 should be fixed now, about Question 2 yeah, the idea was writing something in order to identify each transaction, probably that field should be removed because the smart contracts do not store that info, and at the end, the list of transactions will be retrieved from the backend service.

@apanizo apanizo requested a review from tschubotz May 29, 2018 07:40
@apanizo
Copy link
Contributor Author

apanizo commented May 29, 2018

Travis automatic deployment:
https://staging-31-pr-160-safe-react-gnosis.surge.sh

Storybook book automatic deployment:
https://storybook-staging-31-pr-160-safe-react-gnosis.surge.sh

Copy link
Contributor

@tschubotz tschubotz left a comment

Choose a reason for hiding this comment

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

Thanks for the update, the case with 1 confirmation works now for me!

(4) Another question though:
I tried to test the case where 2 confirmations are needed. Making the transaction works like a charm.

There seems to be a (UI?) bug though. After the tx was successful, it displays "3 confirmations needed" (see screenshot)
image

@apanizo
Copy link
Contributor Author

apanizo commented May 30, 2018

Travis automatic deployment:
https://staging-31-pr-104-safe-react-gnosis.surge.sh

Storybook book automatic deployment:
https://storybook-staging-31-pr-104-safe-react-gnosis.surge.sh

@apanizo
Copy link
Contributor Author

apanizo commented May 30, 2018

@tschubotz A couple of extra fixes implemented. Take a look when you have a chance.

@apanizo apanizo requested a review from tschubotz May 30, 2018 08:12
@apanizo apanizo changed the base branch from feature/WA-238-safe-txs-frontend to feature/WA-238-withdraw-eth May 31, 2018 11:45
@apanizo apanizo merged commit 948196b into feature/WA-238-withdraw-eth May 31, 2018
@apanizo apanizo deleted the feature/WA-238-implementation-multisig-tx branch May 31, 2018 11:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants