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 TipSet identity-producing method to various Node interfaces #149

Merged
merged 6 commits into from Mar 16, 2020

Conversation

laser
Copy link
Contributor

@laser laser commented Mar 13, 2020

This PR makes incremental progress towards completing this ZenHub issue.

Why does this PR exist?

When a provider or client needs information from the chain, they need to provide a chain state identifier to which their query is scoped. Due to potential reorgs, chain height does not serve as identity for a tipset.

What's in this PR?

This PR changes the signature and name of the existing chain state identity-acquiring method, and adds that method to all *Node interfaces which query for (or will query for, in the short future) chain state.

@codecov-io
Copy link

Codecov Report

Merging #149 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #149   +/-   ##
=======================================
  Coverage   82.07%   82.07%           
=======================================
  Files          33       33           
  Lines        1561     1561           
=======================================
  Hits         1281     1281           
  Misses        200      200           
  Partials       80       80
Impacted Files Coverage Δ
storagemarket/types.go 100% <ø> (ø) ⬆️
...oragemarket/impl/providerstates/provider_states.go 97.13% <100%> (ø) ⬆️
storagemarket/impl/storedask/storedask.go 81.43% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d682069...fc73932. Read the comment docs.

@laser laser marked this pull request as ready for review March 13, 2020 22:52
Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

LGTM with one question:

is TipsetKey also a type that lives in StorageMiner? Does that mean rather than live in a shared folder here, it should be elevated to specs-actors (the closed thing to a common type repo) so as not to be duplicated? Similarly, should we consider a "common" node adapter for shared functionality? (probably nothing more than this method ATM)

}
return &TestStateKey{}, n.MostRecentStateIDError

return []byte{}, 0, n.MostRecentStateIDError
Copy link
Collaborator

Choose a reason for hiding this comment

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

we might as well rename this symbol to GetChainHeadError

Copy link
Contributor

@shannonwells shannonwells 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 have no complaints

@laser laser force-pushed the feat/query-chain-using-tipset-key branch from fc73932 to adb9a04 Compare March 16, 2020 15:45
@laser laser merged commit 06e2b48 into master Mar 16, 2020
@laser laser deleted the feat/query-chain-using-tipset-key branch March 16, 2020 15:53
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.

None yet

4 participants