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.

Sign up for GitHub

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

Jump to bottom

Plt 5848 get contract #585

Merged
merged 16 commits into from May 24, 2023
Merged

Plt 5848 get contract #585

merged 16 commits into from May 24, 2023

Conversation

jhbertra
Copy link
Contributor

@jhbertra jhbertra commented May 18, 2023 •
edited

Changes in this PR:

  • Updated and unified query protocol so chain-sync, marlowe-sync, and marlowe-contract all use the same query protocol.
  • Added new contract query API to marlowe-contract
  • Added contract query sub-protocol to marlowe-proxy
  • Added contract store query - get contract by hash
  • Added Runtime CLI query commands

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Test report is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Relevant tickets are mentioned in commit messages
    • Formatting, PNG optimization, etc. are updated
  • PR
    • Self-reviewed the diff
    • Useful pull request description
      • Review required
      • Substantial changes to code, test, or documentation
      • Change made to Marlowe validator (@bwbush and @palas must be included as reviewers)
      • Review not required
      • Minor changes to non-critical code, documentation, nix derivations, configuration files, or scripts
      • Formatting, spelling, grammar, or reorganization
    • Reviewer requested

@jhbertra jhbertra changed the base branch from main to plt-5847-marlowe-loader May 18, 2023 15:33
@jhbertra jhbertra requested review from paluh and bwbush May 24, 2023 15:51
@jhbertra jhbertra self-assigned this May 24, 2023
Base automatically changed from plt-5847-marlowe-loader to main May 24, 2023 17:50
@jhbertra jhbertra marked this pull request as ready for review May 24, 2023 18:13
bwbush
bwbush reviewed May 24, 2023
View reviewed changes
marlowe-integration/src/Test/Integration/Marlowe/Local.hs Outdated
@@ -316,6 +315,7 @@ withLocalMarloweRuntime' MarloweRuntimeOptions{..} test = withRunInIO \runInIO -
contractStore <- createContractStore ContractStoreOptions
{ contractStoreDirectory = resolveWorkspacePath workspace "contract-store"
, contractStoreStagingDirectory = resolveWorkspacePath workspace "contract-staging-area"
, lockingSleepBetweenRetries = 100_000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Presumably this is microseconds. I wonder if lockingMicrosecondsBetweenRetries might be more expressive and self documenting?

bwbush
bwbush reviewed May 24, 2023
View reviewed changes
marlowe-protocols/src/Network/Protocol/Query/Types.hs
resultsVariations :: Tag query delimiter err results -> NonEmpty results

instance ArbitraryQuery query => ArbitraryMessage (Query query) where
class Request req => ArbitraryRequest req where
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why isn't this located in a separate package for test generators. Are these arbitrary instances needed by the protocol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess to avoid orphan instances, it's been like this for quite a long time. presumably dead code elimination should cull this from the final executable

bwbush
bwbush approved these changes May 24, 2023
View reviewed changes
Copy link
Collaborator

@bwbush bwbush left a comment

Choose a reason for hiding this comment

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

Will the format/organization of the file store for contracts always be forward compatible, or do we have to version it? For some users, it might be the only record they have of the continuations, so they'll want it forever usable in new versions of Marlowe Runtime.

@jhbertra
Copy link
Contributor Author

Will the format/organization of the file store for contracts always be forward compatible, or do we have to version it? For some users, it might be the only record they have of the continuations, so they'll want it forever usable in new versions of Marlowe Runtime.

Good question. As long as the .contract files are always forward-compatible, we should be fine. The .closure and .adjacency files are basically just precomputed data from the .contract files. I don't see a reason why the .contract files would ever need to change.

@jhbertra jhbertra merged commit 47e066f into main May 24, 2023
7 checks passed
@jhbertra jhbertra deleted the plt-5848-get-contract branch May 24, 2023 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwbush bwbush bwbush approved these changes

@paluh paluh Awaiting requested review from paluh

Assignees

@jhbertra jhbertra

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jhbertra @bwbush