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

chore: pull abi #145

Closed
wants to merge 3 commits into from
Closed

chore: pull abi #145

wants to merge 3 commits into from

Conversation

mfw78
Copy link
Contributor

@mfw78 mfw78 commented Aug 22, 2023

This PR:

  1. Removes abi artifacts from the repository, instead depending on upstream commits for the latest.

Closes #142

@mfw78 mfw78 self-assigned this Aug 22, 2023
@coveralls
Copy link
Collaborator

Coverage Status

coverage: 81.541%. remained the same when pulling d4e56eb on chore-pull-abi into 2c630af on main.


const UPSTREAM_COMMIT_HASH = '8c762559c98b707801f52dd070dd39ab9478b876'
const UPSTREAM_REPO = 'https://raw.githubusercontent.com/cowprotocol/composable-cow'
const ABI_TO_FETCH = ['ComposableCoW', 'ExtensibleFallbackHandler', 'TWAP']
Copy link
Contributor

Choose a reason for hiding this comment

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

https://raw.githubusercontent.com/cowprotocol/composable-cow/8c762559c98b707801f52dd070dd39ab9478b876/out/ComposableCoW.sol/ComposableCoW.json

The file contains lots of redundant code like:

image

Will it be added to the final bundle? If yes, then it will ad excessive size to frontend consumers.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Yeah, it is in the bundle :(

Could you please trim API files and keep only useful data there?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to load those artifacts async, only if needed to help trim down the bundle?

Comment on lines +9 to +11
const urlsToDownload = ABI_TO_FETCH.map((abi) => {
return `${UPSTREAM_REPO}/${UPSTREAM_COMMIT_HASH}/out/${abi}.sol/${abi}.json`
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to download the abi?
Feels weird to me that's needed.
Usually in such cases, the upstream source publishes a package with the exposed abi files.
Otherwise we pick only the methods we need in a trimmed down version of the abi.

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

I think is not bad to just include a copy of the ABI, and remove any method that is not needed.
Almost similar to how in solidity you just create an interface with the dependency you have with some other contract (and you don't make this interface include all the methods of the contract, but just the ones you need).

@mfw78
Copy link
Contributor Author

mfw78 commented Aug 22, 2023

Closing, with related issue created: cowprotocol/composable-cow#60

@mfw78 mfw78 closed this Aug 22, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 2023
@alfetopito alfetopito deleted the chore-pull-abi branch August 22, 2023 10:30
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.

chore: dynamically pull ABI from upstream
5 participants