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

Banking-app defines its own constitution #53

Merged
merged 35 commits into from
Nov 30, 2022

Conversation

ross-p-smith
Copy link
Contributor

@ross-p-smith ross-p-smith commented Nov 24, 2022

Quite a few changes here for what should have been a simple change.

Why the change

The Banking Sample should define its own constitution rather than copying files over the top of /opt/ccf/bin. We need a change to ccf for this which wont be until v3 microsoft/CCF#4628. This will be updated with #60

The following changes have been made to make the constitution more testable.

  • Moved the bulk of the scripts to a new folder called scripts
  • Added make recipes for testing enclave and virtual in docker (see make help)
  • Ensure test.sh executes irrespective of the hosting model (sandbox/docker)

Todo (in separate PR)

  • Move config and dockerfile into sample folder
  • Fix up the path jumping that you have to do for the certs
  • Find a better way to kill the python process

Test this PR

  • make test - this should build the app, start the sandbox, run the tests, kill the sandbox
  • make test-docker-virtual - this will build the app, build a docker image, start the network, setup the governance and then run the tests

takuro-sato and others added 23 commits October 5, 2022 22:29
* Remove unused codes

* Fix failed CI

Co-authored-by: Takuro Sato <takurosato@microsoft.com>
* Update README.md

* Fix README.md

Co-authored-by: Takuro Sato <takurosato@microsoft.com>
* Add 'why ccf' section

* Add application claim for transfer API

* Update banking-app/README.md

Co-authored-by: Amaury Chamayou <amaury@xargs.fr>

* Update banking-app/README.md

Co-authored-by: Amaury Chamayou <amaury@xargs.fr>

* Update banking-app/test.sh

Co-authored-by: Amaury Chamayou <amaury@xargs.fr>

* Update banking-app/README.md

Co-authored-by: Amaury Chamayou <amaury@xargs.fr>

* Update banking-app/test.sh

Co-authored-by: Amaury Chamayou <amaury@xargs.fr>

* Add 'Get claim' endpoint

* Update /receipt so that the response includes the expanded claim

* Update banking-app/src/endpoints/banking.ts

Co-authored-by: Amaury Chamayou <amaury@xargs.fr>

* Update banking-app/src/endpoints/banking.ts

Co-authored-by: Amaury Chamayou <amaury@xargs.fr>

* Remove /claim

Co-authored-by: Takuro Sato <takurosato@microsoft.com>
Co-authored-by: Amaury Chamayou <amaury@xargs.fr>
Co-authored-by: Takuro Sato <takurosato@microsoft.com>
* Add features to devcontainer

* npm install at container creation
* Run tests in CI

* Tidy up

Co-authored-by: Takuro Sato <takurosato@microsoft.com>
* Add linting

* Fix 'detected dubious ownership in repository' error

* See if CI fails as expected

* Revert "See if CI fails as expected"

This reverts commit 4570982.

Co-authored-by: Takuro Sato <takurosato@microsoft.com>
Co-authored-by: Takuro Sato <takurosato@microsoft.com>
Co-authored-by: Takuro Sato <takurosato@microsoft.com>
Co-authored-by: Takuro Sato <79583855+takuro-sato@users.noreply.github.com>
* Added guide about board

* Typos
* Add simple make commands

* Add initial devcontainer build

* Adding workflow-dispatch to test

* workflow_dispatch

* Add Safe Directory

* Pass GITHUB_WORKSPACE environment variable

* Added image that can use virtual

* Makefile per sample

* Amended CI

* Removed the banking-app clean from the root

Co-authored-by: Takuro Sato <79583855+takuro-sato@users.noreply.github.com>
…oft#40)

* Split demo.sh into demo_governance.sh and demo_application.sh

* add more comments

* remove the extra echo line

* resolve review comments
banking-app/test_docker.sh Outdated Show resolved Hide resolved
banking-app/test_docker.sh Outdated Show resolved Hide resolved
.devcontainer.json Show resolved Hide resolved
banking-app/test_docker.sh Outdated Show resolved Hide resolved
@takuro-sato
Copy link
Contributor

Could you raise an issue to make the same change for auditable-logging-app?

@ross-p-smith
Copy link
Contributor Author

Could you raise an issue to make the same change for auditable-logging-app?

#57

banking-app/Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@julioalex-rezende julioalex-rezende left a comment

Choose a reason for hiding this comment

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

Open to discuss any of the raised comments for a better alignment if necessary.

banking-app/scripts/setup_governance.sh Outdated Show resolved Hide resolved
banking-app/scripts/setup_governance.sh Outdated Show resolved Hide resolved
banking-app/scripts/setup_governance.sh Outdated Show resolved Hide resolved
banking-app/scripts/setup_governance.sh Outdated Show resolved Hide resolved
banking-app/scripts/start_docker.sh Outdated Show resolved Hide resolved
banking-app/scripts/test.sh Outdated Show resolved Hide resolved
config/cchost_config_virtual_js.json Show resolved Hide resolved
Copy link
Contributor

@Aymalla Aymalla left a comment

Choose a reason for hiding this comment

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

LGTM, Just one comment

banking-app/Makefile Show resolved Hide resolved
@ross-p-smith ross-p-smith merged commit f34ae1f into microsoft:main Nov 30, 2022
@ross-p-smith ross-p-smith deleted the ross/constitution branch November 30, 2022 13:49
@ross-p-smith
Copy link
Contributor Author

Fixes #48

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.

4 participants