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.

Sign up for GitHub

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

Jump to bottom

Test add program task #32

Merged
merged 5 commits into from
Nov 14, 2022
Merged

Test add program task #32

merged 5 commits into from
Nov 14, 2022

Conversation

plaintextpaco
Copy link
Collaborator

merge after #31

Main idea behind this PR is to have early feedback on how I'm testing the tasks before tackling all of them, especially the one to launch an aludel.

@itirabasso
Copy link
Collaborator

Could you rebase this PR?

@plaintextpaco
Copy link
Collaborator Author

plaintextpaco commented Nov 8, 2022 •
edited
Loading

Could you rebase this PR?

Done ✨

Awaiting review

@itirabasso
Copy link
Collaborator

This is cool!

I worry we're entangling our tasks too much with hardhat-deploy, but I don't mind it.

itirabasso
itirabasso reviewed Nov 8, 2022
View reviewed changes
tasks/aludel.ts
Comment on lines -201 to -192
task("add-program")
.addParam("aludelFactory", "address of the aludel factory")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could use these tasks for locally deployed contracts, and it would be helpful to overwrite deployed addresses.

I think we need to leave it as an optional param

@plaintextpaco
Copy link
Collaborator Author

I worry we're entangling our tasks too much with hardhat-deploy, but I don't mind it.

the idea behind this is to have the deployments saved in the deployments/ directory so these scripts work with it

We could use these tasks for locally deployed contracts, and it would be helpful to overwrite deployed addresses.
I think we need to leave it as an optional param

If you deploy the contracts locally, then deployments/{localhost,hardhat}
will have the corresponding addresses.

If it's the case that you want to use the tasks on a different deployment, then
you could edit the deployment file.

I feel that being able to use the scripts on addresses that are not registered
anywhere in the repo is a crutch to enable us to not keep track of our
deployments.

@plaintextpaco
removed unused ad-hoc deployment script
4d83aa7
@plaintextpaco
custom log function for tags
bfe9193 
it's meant to log on standalone task runs, but not pollute the test
output
@plaintextpaco
test existing add-program functionality
a5c4889 
well, some of it at least
@plaintextpaco plaintextpaco force-pushed the test-add-program-task branch 2 times, most recently from 4bf7a07 to 51d0cbd Compare November 14, 2022 02:52
@plaintextpaco
failing tests for new implementation of add-program
61b6bc9
@plaintextpaco
add-program remplementation
8213523 
- cleaner logs
- default startTime is now as requested by joeh some time ago
- template is mandatory
- use AludelFactory from deployments
@plaintextpaco plaintextpaco merged commit 823fe29 into main Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itirabasso itirabasso itirabasso approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@plaintextpaco @itirabasso