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 context field to sessions. #278

Merged
merged 20 commits into from Apr 28, 2021
Merged

Conversation

dwedul-figure
Copy link
Contributor

@dwedul-figure dwedul-figure commented Apr 27, 2021

Description

Add a context field to the Session message.

Also add a write-session CLI command and standardize the Use lines for the metadata tx CLI commands based on the cobra documentation.

closes: #276


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@dwedul-figure dwedul-figure added bug Something isn't working metadata Metadata Module labels Apr 27, 2021
@dwedul-figure dwedul-figure self-assigned this Apr 27, 2021
@dwedul-figure dwedul-figure marked this pull request as ready for review April 28, 2021 02:30
@codecov
Copy link

codecov bot commented Apr 28, 2021

Codecov Report

Merging #278 (54bce09) into main (8d84d0f) will increase coverage by 0.27%.
The diff coverage is 84.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #278      +/-   ##
==========================================
+ Coverage   49.97%   50.24%   +0.27%     
==========================================
  Files         119      119              
  Lines       10931    11025      +94     
==========================================
+ Hits         5463     5540      +77     
- Misses       4930     4938       +8     
- Partials      538      547       +9     
Impacted Files Coverage Δ
x/metadata/types/p8e.go 30.53% <50.00%> (+0.13%) ⬆️
x/metadata/client/cli/tx.go 79.49% <85.38%> (-0.03%) ⬇️
x/metadata/handler.go 30.50% <0.00%> (+5.08%) ⬆️

@piercetrey-figure
Copy link

@dwedul-figure I think we will need a way to set this context on the MsgP8eMemorializeContractRequest and have that be set on the session via the conversion

@dwedul-figure
Copy link
Contributor Author

@dwedul-figure I think we will need a way to set this context on the MsgP8eMemorializeContractRequest and have that be set on the session via the conversion

Doh! Yup. Totally forgot that part. Fixed now though. I added it to the Contract message: MsgP8eMemorializeContractRequest.Contract.Context. It's an Any field there too, and it just gets straight up copied over into the Session.Context field.

iramiller
iramiller previously approved these changes Apr 28, 2021
Copy link
Member

@iramiller iramiller left a comment

Choose a reason for hiding this comment

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

One thing to potentially look for is a validation function for that proto any type url ... there are restrictions on the values that are allowed in there per spec... but I wouldn't try to write your own if there isn't one easily usable within the protobuf SDK.

@iramiller
Copy link
Member

iramiller commented Apr 28, 2021

A URL/resource name that uniquely identifies the type of the serialized
protocol buffer message. This string must contain at least
one "/" character. The last segment of the URL's path must represent
the fully qualified name of the type (as in
path/google.protobuf.Duration). The name should be in a canonical form
(e.g., leading "." is not accepted).

In practice, teams usually precompile into the binary all types that they
expect it to use in the context of Any. However, for URLs which use the
scheme http, https, or no scheme, one can optionally set up a type
server that maps type URLs to message definitions as follows:

  • If no scheme is provided, https is assumed.
  • An HTTP GET on the URL must yield a [google.protobuf.Type][]
    value in binary format, or produce an error.
  • Applications are allowed to cache lookup results based on the
    URL, or have them precompiled into a binary to avoid any
    lookup. Therefore, binary compatibility needs to be preserved
    on changes to types. (Use versioned type names to manage
    breaking changes.)

Note: this functionality is not currently available in the official
protobuf release, and it is not used for type URLs beginning with
type.googleapis.com.

Schemes other than http, https (or the empty scheme) might be
used with implementation specific semantics.

source: https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/any.proto

@piercetrey-figure
Copy link

piercetrey-figure commented Apr 28, 2021

I think I am running into issues when sending an Any value related to the stuff @iramiller mentioned above. I have tried several different ways (different prefixes, setting the any type url to github.com/provenance-io/provenance/x/metadata/types/p8e.UUID thinking maybe I had to use the go_package version) and seem to always get something like UNKNOWN: unable to resolve type URL /provenance.metadata.v1.p8e.UUID: tx parse error: invalid request.

Initially I was using a proto from within p8e, but then switched to this UUID proto that is present in the provenance p8e protos, hoping that it would resolve, but no luck so far.

Is there another way I should be specifying the Any type url?

@dwedul-figure
Copy link
Contributor Author

Note for the future: The Cosmos SDK was trying to do some automatic validation of the Any field and failing because it couldn't find the TypeURLs being provided. So we decided to just use a byte array for the Context instead. Since the Session already is already associated with a process, it shouldn't have to tell others what the Context is.

@dwedul-figure dwedul-figure enabled auto-merge (squash) April 28, 2021 23:18
Copy link
Contributor

@channa-figure channa-figure left a comment

Choose a reason for hiding this comment

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

LGTM, I like the edits to the cobra commands.

@dwedul-figure dwedul-figure merged commit eede82d into main Apr 28, 2021
@dwedul-figure dwedul-figure deleted the dwedul/276-session-context branch April 28, 2021 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working metadata Metadata Module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for arbitrary session context state information
4 participants