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

P2: LILA-5460: Add OffsetHelper to SDK #88

Merged

Conversation

GigaHierz
Copy link
Contributor

  • add OffsetHelper,
  • restrict ETH functions for Celo

@toucan-robot toucan-robot changed the title LILA-5460: P2: LILA-5460: Sep 18, 2023
@GigaHierz GigaHierz force-pushed the LILA-5460/add-offset-Helper-to-sdk branch from 42e4590 to 222be79 Compare September 19, 2023 10:20
@GigaHierz GigaHierz changed the title P2: LILA-5460: P2: LILA-5460: Add OffsetHelper to SDK Sep 19, 2023
Copy link
Contributor

@pheuberger pheuberger left a comment

Choose a reason for hiding this comment

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

There's a lot of duplicate things I mentioned. I didn't mention all of them. Just do a project wide search and change all of these :) I didn't review everything yet. Just sending this so you have something to go off of

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/README.md Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
src/subclasses/ContractInteractions.ts Show resolved Hide resolved
src/subclasses/ContractInteractions.ts Show resolved Hide resolved
src/subclasses/ContractInteractions.ts Outdated Show resolved Hide resolved
src/subclasses/ContractInteractions.ts Outdated Show resolved Hide resolved
src/subclasses/ContractInteractions.ts Outdated Show resolved Hide resolved
@GigaHierz GigaHierz force-pushed the LILA-5460/add-offset-Helper-to-sdk branch 2 times, most recently from e6fe6d9 to cca29be Compare October 2, 2023 17:34
Copy link

@Niwilai Niwilai left a comment

Choose a reason for hiding this comment

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

I need to submit current questions and continue with the review

src/index.ts Outdated Show resolved Hide resolved
src/subclasses/ContractInteractions.ts Show resolved Hide resolved
src/subclasses/ContractInteractions.ts Outdated Show resolved Hide resolved
docs/README.md Show resolved Hide resolved
Copy link

@Niwilai Niwilai left a comment

Choose a reason for hiding this comment

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

Looks good, but very hard to review due to a big diff. Some comments:

test/index.test.ts Outdated Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/subclasses/ContractInteractions.ts Outdated Show resolved Hide resolved
src/subclasses/ContractInteractions.ts Show resolved Hide resolved
src/subclasses/ContractInteractions.ts Outdated Show resolved Hide resolved
Copy link

@Niwilai Niwilai left a comment

Choose a reason for hiding this comment

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

Approving here to wish you a farewell and don't drag you around. 👋 However, I will do a follow up PR with 1-2 amends that I still found.

@Niwilai
Copy link

Niwilai commented Oct 12, 2023

@GigaHierz please rebase 🙏

@Niwilai Niwilai dismissed pheuberger’s stale review October 12, 2023 15:13

Agreed with Philipp to dismiss this

@GigaHierz GigaHierz force-pushed the LILA-5460/add-offset-Helper-to-sdk branch from db2725b to 2cfbbd7 Compare October 16, 2023 11:24
@GigaHierz GigaHierz force-pushed the LILA-5460/add-offset-Helper-to-sdk branch from 9116b1c to 49c7b37 Compare October 18, 2023 01:15
@GigaHierz GigaHierz merged commit 37a3f87 into ToucanProtocol:main Oct 18, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants