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 publish rsr to smart contract #383

Merged
merged 70 commits into from
Oct 29, 2024
Merged

Add publish rsr to smart contract #383

merged 70 commits into from
Oct 29, 2024

Conversation

juliangruber
Copy link
Member

@juliangruber juliangruber commented Oct 20, 2024

  • Deploy rsr contract to mainnet
  • Publish rsr
  • Publish own git hash
  • Write tests
  • Create Storacha space
  • Configure Storacha environment variables
  • Update schema
  • Publish as IPLD
  • Publish round details to Storacha
  • Dynamically split CAR into multiple blocks (should not be blocking release)

@juliangruber juliangruber requested a review from bajtos October 20, 2024 12:48
@juliangruber juliangruber changed the title Add publish rsr Add publish rsr to smart contract Oct 20, 2024
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Great start!

Comment on lines +17 to +18
ARG GIT_COMMIT
ENV GIT_COMMIT=$GIT_COMMIT
Copy link
Member

Choose a reason for hiding this comment

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

Should we document this new required argument for the situations when we want to run docker build locally?

Copy link
Member Author

Choose a reason for hiding this comment

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

I never run docker build locally, what is the use case for that?

Copy link
Member

Choose a reason for hiding this comment

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

I usually build locally when troubleshooting docker build failures on the CI or when making changes to the Dockerfile. It's faster to do this locally than wait for the CI run.

It's not a big deal for me if you don't document this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it! I'm happy to document it, where's a good place for that?

bin/spark-evaluate.js Outdated Show resolved Hide resolved
bin/spark-evaluate.js Outdated Show resolved Hide resolved
bin/spark-evaluate.js Outdated Show resolved Hide resolved
lib/publish-rsr.js Outdated Show resolved Hide resolved
lib/publish-rsr.js Outdated Show resolved Hide resolved
lib/publish-rsr.js Outdated Show resolved Hide resolved
lib/publish-rsr.js Outdated Show resolved Hide resolved
lib/publish-rsr.js Outdated Show resolved Hide resolved
lib/publish-rsr.js Outdated Show resolved Hide resolved
@juliangruber juliangruber requested a review from bajtos October 24, 2024 13:30
spark_evaluate_version TEXT NOT NULL,
measurement_commitments TEXT[] NOT NULL,
round_details JSONB NOT NULL,
providers JSONB NOT NULL
Copy link
Member

Choose a reason for hiding this comment

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

I find the name providers as too ambiguous/potentially confusing, since this does not store a list of providers, but a list of per-provider retrieval stats.

The smart-contract uses the name providerRetrievalResultStats, which I find too long and redundant in the scope of unpublished_rsr_rounds.

How about this?

Suggested change
providers JSONB NOT NULL
providers_stats JSONB NOT NULL

Not a big deal, we can keep the current name if you prefer. It's an internal implementation detail that can be easily changed later.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have renamed to provider_retrieval_result_stats, to match the contract. What part of it is redundant in the scope of unpublished_rsr_round?

@juliangruber
Copy link
Member Author

@bajtos hindsight please

@juliangruber juliangruber merged commit 100aa85 into main Oct 29, 2024
6 checks passed
@juliangruber juliangruber deleted the add/publish-rsr branch October 29, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ done
Development

Successfully merging this pull request may close these issues.

2 participants