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

Add an experimental command for importing and exporting collections of contracts #472

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

cmickeyb
Copy link
Contributor

Often a contract is meaningful only in the context of other contracts. For example, an asset issuer contract object may depend upon an asset type contract object. The type object is necessary to establish trust in the scope of the asset being issued.

In order to simplify sharing, a contract collection file packages information about a collection of contracts and their relationship to each other into a single bundle. The file that is created includes a context file that describes the relationships between the contracts and a list of contract description files that can be used to operate on the contract objects.

Note: collection operations are marked as experimental. Any use will generate a warning message when logging is turned on.

Details of the proposed functionality are found in Issue #471.

@cmickeyb cmickeyb added the enhancement New feature or request label Feb 23, 2024
Copy link
Contributor

@g2flyer g2flyer left a comment

Choose a reason for hiding this comment

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

Code looks in general good -- but of course i couldn't resist giving some 5c comments ;-) --. Didn't run it though to check whether everything works as intened. Would be nice to have an executable test case (which would also serve as an example)? Also seems the text from the issue #471 could be useful to add (more or less as-is?) as documentation, in particular as such collection file would be external file-formats?

client/pdo/client/commands/collection.py Outdated Show resolved Hide resolved
client/pdo/client/commands/collection.py Show resolved Hide resolved
client/pdo/client/commands/collection.py Outdated Show resolved Hide resolved
client/pdo/client/commands/collection.py Show resolved Hide resolved
@cmickeyb
Copy link
Contributor Author

#471 could be useful to add (more or less as-is?) as documentation, in particular as such collection file would be external file-formats?

Agreed. Not sure where to put it and wanted to make sure it was kept somewhere

@@ -7,6 +7,7 @@
./pdo/client/builder/shell.py
./pdo/client/builder/state.py
./pdo/client/commands/__init__.py
./pdo/client/commands/collection.py
Copy link
Contributor

@prakashngit prakashngit Mar 2, 2024

Choose a reason for hiding this comment

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

How about updating scripts/Entrypoint.py and setup.py ? (for the 'do_collection' command)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good suggestion. will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

save_files = []
for k, v in context.items() :
if k == 'save_file' :
save_files.append(v)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you are making assumptions about the schema of the context file (by looking for key word 'save_file'). My concern is that the pdo repo AFAIK, does not have any examples of context files. The context files are only introduced in the pdo contracts repo (may be I am wrong?). If this is the case, makes me wonder why we wouldn't maintain some of the client/commands (such as collection, and even context.py) as part of the pdo/contacts repo? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is pretty well documented in the issue so i don't feel a particular need for examples. There is a need for a more thorough test that involves multiple users with completely distinct key sets. We do not currently have the infrastructure to do that. I would suggest that the right way to address this is to add an issue on multi user tests.

Copy link
Member

@bvavala bvavala left a comment

Choose a reason for hiding this comment

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

Suggestion: include the (nice!!) description in #471 as an md file in the client folder.
In fact, this PR adds an experimental support for collections, and #471 nicely describes such support, rather than an issue. The issue could instead be a list of enhancements that need discussion and/or implementation.

cmickeyb added 2 commits March 5, 2024 17:50
…f contracts

Often a contract is meaningful only in the context of other
contracts. For example, an asset issuer contract object may depend
upon an asset type contract object. The type object is necessary to
establish trust in the scope of the asset being issued.

In order to simplify sharing, a contract collection file packages
information about a collection of contracts and their relationship to
each other into a single bundle. The file that is created includes a
context file that describes the relationships between the contracts
and a list of contract description files that can be used to operate
on the contract objects.

Signed-off-by: Mic Bowman <[email protected]>
Added documentation (client/docs/collection.md).

Added entrypoint for a collection command.

Updated types.

Signed-off-by: Mic Bowman <[email protected]>
@cmickeyb cmickeyb force-pushed the mic.feb20.collections branch from 1e3aa47 to 903ec71 Compare March 6, 2024 01:51
Copy link
Contributor

@prakashngit prakashngit left a comment

Choose a reason for hiding this comment

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

Thanks for the new commit to address the feedback. I am good with the PR.

@bvavala bvavala merged commit 1dee676 into hyperledger-labs:main Mar 6, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants