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

test tube and ci #13

Merged
merged 7 commits into from
Sep 25, 2023
Merged

test tube and ci #13

merged 7 commits into from
Sep 25, 2023

Conversation

keyleu
Copy link
Collaborator

@keyleu keyleu commented Sep 21, 2023

Description

Added CI for testing the contract using the test-tube.
Added tests to initial version of contract.

Reviewers checklist:

  • Try to write more meaningful comments with clear actions to be taken.
  • Nit-picking should be unblocking. Focus on core issues.

Authors checklist

  • Provide a concise and meaningful description
  • Review the code yourself first, before making the PR.
  • Annotate your PR in places that require explanation.
  • Think and try to split the PR to smaller PR if it is big.

This change is Reviewable

@keyleu keyleu requested a review from a team as a code owner September 21, 2023 12:29
@keyleu keyleu requested review from dzmitryhil, miladz68, ysv and wojtek-coreum and removed request for a team September 21, 2023 12:29
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @keyleu, @wojtek-coreum, and @ysv)


.github/workflows/contract-ci.yml line 8 at r1 (raw file):

    pull_request:
      branches: ["master"]
      

this empty line should not exist


.github/workflows/contract-ci.yml line 20 at r1 (raw file):

    steps:
      - uses: actions/checkout@v3
      

same here


.github/workflows/contract-ci.yml line 38 at r1 (raw file):

      - name: Run tests
        run: cargo test --verbose
        working-directory: contract

empty line needed

Copy link
Collaborator Author

@keyleu keyleu left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)


.github/workflows/contract-ci.yml line 8 at r1 (raw file):

Previously, miladz68 (milad) wrote…

this empty line should not exist

removed


.github/workflows/contract-ci.yml line 20 at r1 (raw file):

Previously, miladz68 (milad) wrote…

same here

removed


.github/workflows/contract-ci.yml line 38 at r1 (raw file):

Previously, miladz68 (milad) wrote…

empty line needed

added

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 8 files reviewed, 7 unresolved discussions (waiting on @keyleu, @miladz68, @wojtek-coreum, and @ysv)


.github/workflows/contract-ci.yml line 22 at r2 (raw file):

        run: |
          cd contract
          docker run --rm -v "$(pwd)":/code \

Please add todo to update it to make file command.


.github/workflows/contract-ci.yml line 26 at r2 (raw file):

            --mount type=volume,source=registry_cache,target=/usr/local/cargo/registry \
            cosmwasm/rust-optimizer:0.14.0
      - name: Set up cargo cache

Why do you call cache after the optimize step?


contract/src/tests.rs line 41 at r2 (raw file):

            None,
            "label".into(),
            &coins(10_000_000, FEE_DENOM),

Why do you need that? The admin should attach the coins for the token issuance and exec amount needed for it.


contract/src/tests.rs line 101 at r2 (raw file):

    #[test]
    fn transfer_ownership() {

Is it possible by the ownership contract to withdraw the coins from the contract? If yes, we need to discuss with the team, probably we should prohibit it. And in general what is allowed to be done by the admin?

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 8 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @keyleu, @miladz68, @wojtek-coreum, and @ysv)

Copy link
Collaborator Author

@keyleu keyleu left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 8 files reviewed, 7 unresolved discussions (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)


.github/workflows/contract-ci.yml line 22 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Please add todo to update it to make file command.

done


.github/workflows/contract-ci.yml line 26 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why do you call cache after the optimize step?

right, this is a mistake, I removed it.


contract/src/tests.rs line 41 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why do you need that? The admin should attach the coins for the token issuance and exec amount needed for it.

that's what this does, attach the coins. If "signer" doesn't have this amount to attach, it will fail.


contract/src/tests.rs line 101 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Is it possible by the ownership contract to withdraw the coins from the contract? If yes, we need to discuss with the team, probably we should prohibit it. And in general what is allowed to be done by the admin?

This is not really an admin, it's more an OWNER.
const OWNERSHIP: Item<Ownership> = Item::new("ownership");

The owner will be able to do the actions that we define for him inside the contract. He won't be able to withdraw the coins unless we make a specific function to do that, so we have full control on what the owner can and can't do.

The admin of the contract is the one that can migrate the contract and is the one you define when you instantiate the contract. This admin can do anything. He can potentially make a new contract with withdraw all coins and then migrate it and empty it. But this is totally different to this Owner that we are using here.

dzmitryhil
dzmitryhil previously approved these changes Sep 22, 2023
Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 8 files reviewed, 3 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @keyleu, @wojtek-coreum, and @ysv)


.github/workflows/contract-ci.yml line 36 at r3 (raw file):

        run: cargo test --verbose
        working-directory: contract
        

the added line has empty spaces in it. github is still complaining (notice the red marking by github)

Copy link
Collaborator Author

@keyleu keyleu left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 10 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)


.github/workflows/contract-ci.yml line 36 at r3 (raw file):

Previously, miladz68 (milad) wrote…

the added line has empty spaces in it. github is still complaining (notice the red marking by github)

done, should be fine now

miladz68
miladz68 previously approved these changes Sep 25, 2023
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @keyleu, @wojtek-coreum, and @ysv)


contract/src/state.rs line 12 at r4 (raw file):

    TokensCoreum = b'1',
    TokensXRPL = b'2',
    XRPLCurrencies = b'X',

Why do you need X and C here? IMO 3,4 is is good enough.

Copy link
Collaborator Author

@keyleu keyleu left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)


contract/src/state.rs line 12 at r4 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why do you need X and C here? IMO 3,4 is is good enough.

it doesn't really matter what I use here, I just need any byte that isn't repeated, I thought X for XRPL and C for Coreum was fair enough but if you prefer 3 and 4 we can use those.
Changed it.

Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum and @ysv)

@keyleu keyleu merged commit 41f6359 into master Sep 25, 2023
4 checks passed
@keyleu keyleu deleted the keyne/test-tube-and-ci branch January 10, 2024 09:05
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