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

feat(x/data)!: enable off-chain coordination of supported algorithms #2102

Merged
merged 25 commits into from
Jan 30, 2024

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Dec 7, 2023

Description

This PR introduces regen.data.v2 which:

  1. does not validate digest, canonicalization or merkle tree algorithms
  2. does not use an enum for raw data extensions

This allows off-chain coordination of these values so that extensions can be made without requiring an upgrade.

regen/data/v2/types.proto SHOULD still be used as a registry of agreed upon algorithms, but this can be done between releases without requiring chain upgrades.

This PR deprecates the regen.data.v1 proto files and bumps the x/data go module to v3.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@aaronc aaronc changed the title feat(x/data): enable off-chain coordination of media extensions, dige… feat(x/data): enable off-chain coordination of supported algorithms Dec 7, 2023
scripts/protocgen.sh Outdated Show resolved Hide resolved
scripts/protocgen2.sh Outdated Show resolved Hide resolved
@aaronc aaronc marked this pull request as ready for review December 7, 2023 19:02
Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

Looks good, just a few nits and pending lint errors
Should I just use version 2 moving forward in regen-server/iri-gen, in particular in my PR: https://github.com/regen-network/regen-server/pull/423

proto/regen/data/v1/types.proto Outdated Show resolved Hide resolved
proto/regen/data/v1/types.proto Outdated Show resolved Hide resolved
x/data/iri_test.go Outdated Show resolved Hide resolved
@aaronc
Copy link
Member Author

aaronc commented Dec 11, 2023

Looks good, just a few nits and pending lint errors

Should I just use version 2 moving forward in regen-server/iri-gen, in particular in my PR: https://github.com/regen-network/regen-server/pull/423

We have no idea how long this will take to go into production so for now we need to stick with v1.

@clevinson
Copy link
Member

Thinking about the long-term effects of a versioning name change like this, I'm wondering if we'd be better off with one of the following:

  1. using the pre-existing versioning system given by the protocol paths (e.g. just bumping this to proto/regen/data/v2..)
  2. Updating the existing implementation of the current Graph and Raw types (in promo/regen/data/v1...) to match the desired implementation (what this PR current does in GraphV2 and RawV2).

Conceptually I'm in favor of this general approach (not validating enums on-chain to enable for faster developing & off-chain coordination). But given that we have such little usage of the data module to date, I think we may be able to skirt around what would otherwise be the strictly correct process for API upgrades, in favor of something that's a bit cleaner for future users.

@aaronc
Copy link
Member Author

aaronc commented Dec 12, 2023

Thinking about the long-term effects of a versioning name change like this, I'm wondering if we'd be better off with one of the following:

  1. using the pre-existing versioning system given by the protocol paths (e.g. just bumping this to proto/regen/data/v2..)
  2. Updating the existing implementation of the current Graph and Raw types (in promo/regen/data/v1...) to match the desired implementation (what this PR current does in GraphV2 and RawV2).

I would be open to either approach - and I think this would greatly simply the implementation. I've checked data module events on mainnet and so far there is no usage. So until we start using it (which is soon) breakage isn't really a concern. With approach 1) we would simply stop supporting v1 I think to keep things simple. Approach 2) is maybe okay since there are no other users but it is a bit un-kosher. Don't have any really strong opinions between the two though. @blushi any thoughts?

@blushi
Copy link
Member

blushi commented Dec 12, 2023

Thinking about the long-term effects of a versioning name change like this, I'm wondering if we'd be better off with one of the following:

  1. using the pre-existing versioning system given by the protocol paths (e.g. just bumping this to proto/regen/data/v2..)
  2. Updating the existing implementation of the current Graph and Raw types (in promo/regen/data/v1...) to match the desired implementation (what this PR current does in GraphV2 and RawV2).

I would be open to either approach - and I think this would greatly simply the implementation. I've checked data module events on mainnet and so far there is no usage. So until we start using it (which is soon) breakage isn't really a concern. With approach 1) we would simply stop supporting v1 I think to keep things simple. Approach 2) is maybe okay since there are no other users but it is a bit un-kosher. Don't have any really strong opinions between the two though. @blushi any thoughts?

I think I'd rather go with approach 1) because v1 hasn't been explicitly used on mainnet but it has been in our implementation within the regen server for generating IRIs for credit classes, projects and batches metadata (and might be used for raw data IRIs depending on the timeline) so that would feel cleaner to have a proper version upgrade.

@aaronc
Copy link
Member Author

aaronc commented Dec 12, 2023

Okay. I'll update this PR to change the proto version to v2 and drop support for v1, unless there are any objections

@aaronc aaronc changed the title feat(x/data): enable off-chain coordination of supported algorithms feat(x/data)!: enable off-chain coordination of supported algorithms Dec 12, 2023
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0936200) 73.26% compared to head (9184fe0) 73.26%.
Report is 1 commits behind head on main.

❗ Current head 9184fe0 differs from pull request most recent head 1320571. Consider uploading reports for the commit 1320571 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2102   +/-   ##
=======================================
  Coverage   73.26%   73.26%           
=======================================
  Files         232      232           
  Lines       13748    13748           
=======================================
  Hits        10073    10073           
  Misses       2941     2941           
  Partials      734      734           
Files Coverage Δ
app/app.go 92.68% <ø> (ø)
app/upgrades/v5_0/upgrade.go 78.12% <ø> (ø)
x/data/client/testsuite/util.go 100.00% <100.00%> (ø)
x/data/server/msg_anchor.go 58.46% <ø> (ø)
x/data/server/msg_attest.go 65.11% <ø> (ø)
x/data/server/msg_define_resolver.go 66.66% <ø> (ø)
x/data/server/msg_register_resolver.go 68.42% <ø> (ø)
x/data/server/query_anchor_by_hash.go 86.95% <ø> (ø)
x/data/server/query_anchor_by_iri.go 86.95% <ø> (ø)
x/data/server/query_attestations_by_attestor.go 66.66% <ø> (ø)
... and 8 more

@aaronc
Copy link
Member Author

aaronc commented Dec 12, 2023

@clevinson @blushi I've updated this to v2 (and the go.mod to v3) and it's R4R again. The diff is much larger because of breaking changes, but the code changes are actually somewhat simpler.

proto/regen/data/v2/types.proto Outdated Show resolved Hide resolved
x/data/iri.go Outdated Show resolved Hide resolved
x/data/iri.go Outdated Show resolved Hide resolved
x/data/iri.go Outdated Show resolved Hide resolved
x/data/iri.go Outdated Show resolved Hide resolved
x/data/iri.go Show resolved Hide resolved
Co-authored-by: Marie Gauthier <[email protected]>
@aaronc
Copy link
Member Author

aaronc commented Dec 13, 2023

@blushi I've addressed all your comments, except that I'd prefer to keep the IRI on version 0 and updated the comments to reflect that.

Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

LGTM, pre-approving, pending one test case to add

x/data/types.go Show resolved Hide resolved
@aaronc
Copy link
Member Author

aaronc commented Jan 7, 2024

@clevinson could you take another quick look at this?

Copy link
Member

@clevinson clevinson left a comment

Choose a reason for hiding this comment

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

Sorry this took so long. lgtm!

@aaronc aaronc enabled auto-merge (squash) January 25, 2024 23:59
@aaronc aaronc merged commit 53ed6d2 into main Jan 30, 2024
25 of 26 checks passed
@aaronc aaronc deleted the aaronc/data-off-chain-enums branch January 30, 2024 00:59
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