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

Sync wrappers with underlying contracts #77

Merged
merged 1 commit into from
Jan 5, 2022
Merged

Conversation

ermyas
Copy link
Contributor

@ermyas ermyas commented Jan 5, 2022

No description provided.

@ermyas ermyas requested a review from drinkcoffee January 5, 2022 22:35
@ermyas ermyas merged commit eaf6c40 into main Jan 5, 2022
@ermyas ermyas deleted the fix/out-of-sync-wrapper branch January 5, 2022 23:15
@ricardolyn
Copy link

Note: it might be good for the future to remove generated code from the repo and automatically do it as part of the build

@ermyas
Copy link
Contributor Author

ermyas commented Jan 6, 2022

I agree

@drinkcoffee
Copy link
Contributor

However, then when people clone the repo to view the code, their IDE will complain with lots of static analysis errors, as source code won't exist. This will lead some people to conclude that the code in the repo is broken.

@ermyas
Copy link
Contributor Author

ermyas commented Jan 7, 2022

I don't think that will be too much of an issue.
Our ReadMe can indicate that you need to run a Gradle build (or at least trigger the generator task), which isn't an uncommon practice, so I don't imagine most people will be stumped by this. However, at the moment our Gradle build triggers both unit and integration tests, the latter of which requires a test network to have been spun up apriori and takes a fair bit of time to execute. This makes things a bit more onerous for a person that just wants to explore the codebase. The solution to this, however, I think is a combination of a) separating the running of our integration tests from the standard build task, and b) using hardhat to minimise the overheads of the test network.

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.

3 participants