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

fix lotus-miner storage-deals set-ask - make sure that ActorAddress works on all miner nodes #6939

Closed
wants to merge 2 commits into from

Conversation

nonsense
Copy link
Member

This PR is fixing #6921

@nonsense nonsense requested a review from a team as a code owner July 29, 2021 10:15
@nonsense nonsense changed the title fix lotus-miner storage-deals set-as - remove calls to miner service; use input from user fix lotus-miner storage-deals set-ask - remove calls to miner service; use input from user Jul 29, 2021
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

I'd expect ActorAddress to work on the market node, with that we can call the full node for sector size -> StateMinerInfo().SectorSize

@nonsense nonsense force-pushed the nonsense/fix-set-ask-for-mra branch from 7a29872 to 4dee5d8 Compare July 29, 2021 10:58
@nonsense nonsense changed the title fix lotus-miner storage-deals set-ask - remove calls to miner service; use input from user fix lotus-miner storage-deals set-ask - make sure that ActorAddress works on all miner nodes Jul 29, 2021
@jacobheun jacobheun added epic/MRA release/backport team/ignite Issues and PRs being tracked by Team Ignite at Protocol Labs labels Jul 29, 2021
@jacobheun jacobheun added this to the 1.11.1 milestone Jul 29, 2021
@nonsense
Copy link
Member Author

@raulk @magik6k given that we discussed that this is an immutable value, I think this is good solution. You okay with it, or you have something else in mind?

@nonsense nonsense requested a review from raulk July 29, 2021 13:45
@jacobheun jacobheun added P2 P2: Should be resolved P1 P1: Must be resolved and removed P2 P2: Should be resolved labels Jul 29, 2021
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Honestly I'm not comfortable doing this. The reason why this works is because it's a side-effect from having copied the miner's metadata store into the markets process. I'm not even sure that was the right thing to do in the first place, and if we merge this, we will be marrying to that approach forever.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

This is already solved by #6936, as CLI commands are now intelligent to route to the miner node or the markets node. I don't see the need to merge a miner-side change.

@jacobheun
Copy link
Contributor

Closing as this is no longer needed with #6936.

@jacobheun jacobheun closed this Jul 29, 2021
@Kubuxu Kubuxu deleted the nonsense/fix-set-ask-for-mra branch November 25, 2021 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 P1: Must be resolved team/ignite Issues and PRs being tracked by Team Ignite at Protocol Labs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants