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

feat: added sourcify with tests #879

Merged
merged 4 commits into from May 19, 2022

Conversation

0xvangrim
Copy link
Contributor

Description

Issue: #755

Adding a Sourcify API call to see if the contract chosen is verified or not. If it is chosen, we will decode the ABI and give the user an interface.

image

Difficult to test, but I added tests and tested it so it doesn't break any of the existing logic we use to show the contract interface.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • [X ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Manually and with unit tests.

Checklist:

  • [X ] My code follows the existing style of this project
  • [X ] I have performed a self-review of my own code
  • [ X] I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • [ X] I have added tests that prove my fix is effective or that my feature works
  • [X ] New and existing unit tests pass locally with my changes
  • Any UI changes have been tested and made responsive for mobile views

@github-actions
Copy link

✔️ Preview deployment is ready!

🔨 Explore the source changes: 82fde4b

😎 Browse the preview: https://bafybeiarb35g6flwo6havr2jut6wmm5paqkn3u33osift4vgng2kevrsnu.ipfs.dweb.link

@github-actions
Copy link

✔️ Preview deployment is ready!

🔨 Explore the source changes: 5f8868c

😎 Browse the preview: https://bafybeib2ik6rjpbqc5nfex6huwmabe2eotwdownuelpfudlj4m37zcmzwq.ipfs.dweb.link

@0xvangrim 0xvangrim marked this pull request as ready for review May 18, 2022 00:29
MiltonTulli
MiltonTulli previously approved these changes May 18, 2022
};
}, [call, contracts, chainId]);

return decodedCall || { decodedCall: null, contract: null };
Copy link
Member

Choose a reason for hiding this comment

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

missing approval in the fallback object.

@@ -0,0 +1,10 @@
export const lookUpContractWithSourcify = async ({ chainId, address }) => {
const baseUrl = `https://sourcify.dev/server/files/any`;
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we store this in env/configs/constants file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sourcify will be used to lookup contracts that we are not storing ourselves. Or am I missing something in your question?

Copy link
Member

Choose a reason for hiding this comment

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

Just wondering if we should pull this url from config file, or env. Nothing serious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see, now I understand!

Copy link
Member

@AugustoL AugustoL left a comment

Choose a reason for hiding this comment

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

The only thing that It is missing is a test comparing a the abi and contact byecode hash from the richContractData with one from sourcify, I see you are only testing against a random contract/

@0xvangrim
Copy link
Contributor Author

0xvangrim commented May 18, 2022

The only thing that It is missing is a test comparing a the abi and contact byecode hash from the richContractData with one from sourcify, I see you are only testing against a random contract/

Hm, I'm not certain why we would need to have that test? The richContractData and sourcify should never overlap. When the contract is not in the contract registry we will fetch it from sourcify, but only then. We will never make a double-check against both the registry and sourcify.

@0xvangrim 0xvangrim requested a review from AugustoL May 19, 2022 07:47
@0xvangrim
Copy link
Contributor Author

@AugustoL I've tested some of our richContractData that exists in our dxvote-file now and they haven't been been verified there yet.

@AugustoL
Copy link
Member

Approved, we see how we can add those tests for sourcify after then

@0xvangrim 0xvangrim merged commit 991a5d0 into DXgovernance:v1.1-beta May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants