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

Remove unnecessary transaction proposal #3269

Merged
merged 3 commits into from
Jan 10, 2022
Merged

Conversation

iamacook
Copy link
Member

What it solves

Resolves #3261

How this PR fixes it

Transactions need only be proposed if it is not immediately executing. The unnecessary proposal has been removed.

How to test it

  1. Create a transaction and ensure that it has sufficient confirmations, but do not execute it.
  2. Execute it as a non-owner and expect it to be successful and not show an error in the console.

@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Jan 10, 2022

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@iamacook iamacook requested a review from mmv08 January 10, 2022 15:36
@iamacook iamacook marked this pull request as ready for review January 10, 2022 15:36
}

dispatch(fetchTransactions(chainId, safeAddress))
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to fetch transactions immediately if it was an execution because it will take time for the indexer to pick up the transaction. So the minimum would be the block time (10s).

Copy link
Member Author

Choose a reason for hiding this comment

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

@katspaugh and I are in the process of doing a huge refactor of this and it is already removed there, but I'll add it here for good measure.

@coveralls
Copy link

coveralls commented Jan 10, 2022

Pull Request Test Coverage Report for Build 1678175832

  • 0 of 5 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 32.43%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/logic/safe/store/actions/processTransaction.ts 0 5 0.0%
Totals Coverage Status
Change from base Build 1677658745: 0.0%
Covered Lines: 3113
Relevant Lines: 8552

💛 - Coveralls

@github-actions
Copy link

Deployment links

🟠 Safe Rinkeby Safe Mainnet 🟣 Safe Polygon 🟡 Safe BSC Safe Arbitrum 🟢 Safe xDai

@github-actions
Copy link

E2E Tests Failed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1678208473

Failed tests:

  • ❌ Read-only transaction creation and review Read-only transaction creation and review
  • ❌ Safe Balances Safe Balances

@francovenica
Copy link
Contributor

Works fine for me. I don't have the same error in the console that it was reported.
The tx went through just fine as well

@iamacook iamacook merged commit 7fba9d2 into dev Jan 10, 2022
@iamacook iamacook deleted the non-owner-error-message branch January 10, 2022 18:07
@github-actions github-actions bot locked and limited conversation to collaborators Jan 10, 2022
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.

Error with all owners and delegates for the safe when tx executed by non-owner
5 participants