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

Add nested delegate call warnings #3122

Merged
merged 3 commits into from
Dec 8, 2021
Merged

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Dec 6, 2021

What it solves

Resolves #3115

How this PR fixes it

Nested delegate calls now display that they are a) a delegate call and b) (un-)expected. They also, like #3091, base their legitimacy on whether the address is known or not.

An unexpected, nested delegate call can be found here.

How to test it

The linked example transaction can be used as a PoC for this new warning.

In order to create another, it needs to be done via a script. You would have to use node.js or propose a transaction.

Screenshots

(The following "parent" delegate calls are "unexpected" because our MultiSend contract is not a known address on Rinkeby.)

image

The known address name was set manually here in order to trigger the warning:
image

@iamacook iamacook marked this pull request as draft December 6, 2021 14:37
@github-actions
Copy link

github-actions bot commented Dec 6, 2021

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Dec 6, 2021

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

@github-actions
Copy link

github-actions bot commented Dec 6, 2021

Deployment links

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

@coveralls
Copy link

coveralls commented Dec 6, 2021

Pull Request Test Coverage Report for Build 1545311346

  • 1 of 6 (16.67%) changed or added relevant lines in 2 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.02%) to 32.473%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/routes/safe/components/Transactions/TxList/MultiSendDetails.tsx 0 2 0.0%
src/routes/safe/components/Transactions/TxList/DelegateCallWarning.tsx 1 4 25.0%
Files with Coverage Reduction New Missed Lines %
src/routes/safe/components/Transactions/TxList/MultiSendDetails.tsx 1 4.65%
src/components/App/ReceiveModal.tsx 4 0%
Totals Coverage Status
Change from base Build 1544278291: -0.02%
Covered Lines: 3087
Relevant Lines: 8471

💛 - Coveralls

@iamacook iamacook marked this pull request as ready for review December 6, 2021 14:48
@github-actions
Copy link

github-actions bot commented Dec 6, 2021

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

Failed tests:

  • ❌ Add an existing safe Add an existing safe
  • ❌ Address book Address book
  • ❌ Safe Apps List Safe Apps List
  • ❌ Read-only transaction creation and review Create and review a Send Funds transaction

@katspaugh
Copy link
Member

Can we remove the extra gap and add a gap in the action section?

Screenshot 2021-12-06 at 16 20 15

@francovenica
Copy link
Contributor

I don't have a way to reproduce a tx like the one in the example.
The tx given as example works fine in this PR and shows the warning message so I'm ok with passing this ticket.
@katspaugh Do you think we should try to find a contract and reproduce a similar tx?

@iamacook
Copy link
Member Author

iamacook commented Dec 8, 2021

Merging as confirmed to work by @mikheevm.

@iamacook iamacook merged commit f7af433 into dev Dec 8, 2021
@iamacook iamacook deleted the add-nested-delegate-call-warnings branch December 8, 2021 13:45
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2021
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.

Add explicit warning about nested delegate calls
5 participants