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

feat(collector): add multi-token support #119

Merged
merged 16 commits into from
Feb 20, 2023
Merged

feat(collector): add multi-token support #119

merged 16 commits into from
Feb 20, 2023

Conversation

jurajpiar
Copy link
Member

@jurajpiar jurajpiar commented Feb 7, 2023 •
edited

What

  1. Creates new multi-token collector contract Modifies Collector contract to allow multiple tokens
  2. adds generated files to gitignore
  3. removes unnecessary loops in the Collector contract
  4. enables compilation in the TDD mode
  5. adds contracts and tasks to the watch mode

Refs

@jurajpiar jurajpiar marked this pull request as draft February 7, 2023 16:14
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 9, 2023
View reviewed changes
test/MultitokenCollector.test.ts Fixed Show fixed Hide fixed
test/MultitokenCollector.test.ts Fixed Show fixed Hide fixed
@jurajpiar jurajpiar force-pushed the PP-664/multitoken_collector branch 4 times, most recently from 7f3a5cc to 9136938 Compare February 9, 2023 21:34
@jurajpiar jurajpiar marked this pull request as ready for review February 9, 2023 21:36
@jurajpiar jurajpiar force-pushed the PP-664/multitoken_collector branch 2 times, most recently from 34e3760 to 255c1c7 Compare February 9, 2023 21:52
@jurajpiar jurajpiar requested a review from a team February 10, 2023 09:32
@franciscotobar
Copy link
Contributor

while executing the npm run test, Im getting the following warning Failed to generate 2 stack traces. Run Hardhat with --verbose to learn more., before this warning was not showing.

franciscotobar
franciscotobar approved these changes Feb 12, 2023
View reviewed changes
Copy link
Contributor

@franciscotobar franciscotobar left a comment

Choose a reason for hiding this comment

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

LGTM

antomor
antomor requested changes Feb 13, 2023
View reviewed changes
Copy link
Collaborator

@antomor antomor left a comment

Choose a reason for hiding this comment

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

@jurajpiar Thank you for the changes, they look good apart from some minor comments I left.
Furthermore, I think we should change deploy-collector.input.sample.json and also the analysis document to reflect the new changes.

test/tasks/deployCollector.test.ts Show resolved Hide resolved
test/Collector.test.ts Show resolved Hide resolved
test/Collector.test.ts Show resolved Hide resolved
test/Collector.test.ts Outdated Show resolved Hide resolved
test/Collector.test.ts Outdated Show resolved Hide resolved
test/Collector.test.ts Outdated Show resolved Hide resolved
test/Collector.test.ts Outdated Show resolved Hide resolved
@antomor
Copy link
Collaborator

antomor commented Feb 13, 2023

while executing the npm run test, Im getting the following warning Failed to generate 2 stack traces. Run Hardhat with --verbose to learn more., before this warning was not showing.

@jurajpiar do we know the reason behind it?

@antomor
Copy link
Collaborator

antomor commented Feb 13, 2023 •
edited

Furthermore, I think that we should add the functions to add/remove tokens from the collector

ulasanil
ulasanil reviewed Feb 13, 2023
View reviewed changes
contracts/Collector.sol Show resolved Hide resolved
contracts/Collector.sol Show resolved Hide resolved
antomor
antomor requested changes Feb 14, 2023
View reviewed changes
contracts/Collector.sol Outdated Show resolved Hide resolved
test/tasks/deployCollector.test.ts Show resolved Hide resolved
contracts/Collector.sol Show resolved Hide resolved
ulasanil
ulasanil approved these changes Feb 14, 2023
View reviewed changes
Copy link
Contributor

@ulasanil ulasanil left a comment

Choose a reason for hiding this comment

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

LGTM!

@ironFe93
Copy link
Contributor

@antomor added a token address parameter for the removeToken() function, fixed tests and made changes to the input file and deploy file. Also modified the document analysis.

ironFe93
ironFe93 approved these changes Feb 17, 2023
View reviewed changes
Copy link
Contributor

@ironFe93 ironFe93 left a comment

Choose a reason for hiding this comment

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

LGTM

jurajpiar
jurajpiar commented Feb 20, 2023
View reviewed changes
tasks/deployCollector.ts Outdated Show resolved Hide resolved
jurajpiar
jurajpiar commented Feb 20, 2023
View reviewed changes
test/Collector.test.ts Outdated Show resolved Hide resolved
jurajpiar
jurajpiar commented Feb 20, 2023
View reviewed changes
test/Collector.test.ts Outdated Show resolved Hide resolved
antomor
antomor approved these changes Feb 20, 2023
View reviewed changes
Copy link
Collaborator

@antomor antomor left a comment

Choose a reason for hiding this comment

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

LGTM

@antomor antomor merged commit 69a334b into main Feb 20, 2023
@antomor antomor deleted the PP-664/multitoken_collector branch February 20, 2023 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antomor antomor antomor approved these changes

@ironFe93 ironFe93 ironFe93 approved these changes

@franciscotobar franciscotobar franciscotobar approved these changes

@ulasanil ulasanil ulasanil 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.

None yet
5 participants
@jurajpiar @franciscotobar @antomor @ironFe93 @ulasanil