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

cadence 1.0 upgrade #1

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

cadence 1.0 upgrade #1

wants to merge 7 commits into from

Conversation

dryruner
Copy link
Contributor

@dryruner dryruner commented Apr 1, 2024

No description provided.

Copy link

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

Just reviewed some of the contracts that looked like they were important. I didn't get to any of the transactions yet. Can you do two things before we review any more of your code?

  • Check out the comment that I made about entitlements and make sure that you've gone through all your code in the four PRs you shared with us and added entitlements to all the functions that need them.
  • In each PR, let us know which contracts are yours that need our review. It was not clear in this PR which contracts that I should review and which were just dependencies that you have no control over

Thank you!

src/contracts/SwapFactory.cdc Outdated Show resolved Hide resolved
src/contracts/SwapFactory.cdc Outdated Show resolved Hide resolved
src/contracts/SwapFactory.cdc Outdated Show resolved Hide resolved
src/contracts/SwapFactory.cdc Outdated Show resolved Hide resolved
src/contracts/SwapFactory.cdc Outdated Show resolved Hide resolved
src/contracts/env/FlowSwapPair.cdc Show resolved Hide resolved
src/contracts/env/MultiFungibleToken.cdc Show resolved Hide resolved
src/contracts/env/MultiFungibleToken.cdc Outdated Show resolved Hide resolved
src/contracts/env/MultiFungibleToken.cdc Outdated Show resolved Hide resolved
src/contracts/env/OnChainMultiSig.cdc Outdated Show resolved Hide resolved
@dryruner
Copy link
Contributor Author

dryruner commented Apr 29, 2024

Hello @joshuahannan Please take another look! Updated according to your comments and applied correct auth model.

In-Scope:

  • src/*.cdc; excluding src/env/* and src/tokens/* as those are all dependencies (FT standards, dummy tokens, blocto code, etc.)

@joshuahannan
Copy link

What about all of the swap pair contracts? Those all seem to be copies of each other with just a new names changed, so can we just review one of them and you can propagate all the changes to the others?

@dryruner
Copy link
Contributor Author

What about all of the swap pair contracts? Those all seem to be copies of each other with just a new names changed, so can we just review one of them and you can propagate all the changes to the others?

Yes all will be upgraded with the same template file.

@joshuahannan
Copy link

Great, which template file should we be reviewing? Also, can you go through the other PRs and make sure the entitlements are correct and provide the same guidance for all the other PRs you asked us to review concerning which contracts to review and which to pass on? Let us know in a comment on each PR when that is ready

@dryruner
Copy link
Contributor Author

dryruner commented May 2, 2024

template file

The template file is SwapPair.cdc

make sure the entitlements are correct

Yeah for sure, working on that and will inform you once done, thanks for the review!

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.

None yet

2 participants