-
Notifications
You must be signed in to change notification settings - Fork 153
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
refactor: migrate MinerGetBaseInfo, StateCall to trait RpcMethod #4244
Conversation
|
||
async fn handle( | ||
ctx: Ctx<impl Blockstore + Send + Sync + 'static>, | ||
(LotusJson(message), LotusJson(ApiTipsetKey(tsk))): Self::Params, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hope we can get rid of LotusJson
wrappers in Params
as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hope we can get rid of
LotusJson
wrappers inParams
as well
Yes, exactly what I was thinking. It's quite verbose both here and in api compare
tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aatifsyed I made a draft PR #4247 to drop these wrappers, we can use it for discussion.
shared_tipset.key().into(), | ||
))); | ||
tests.push(RpcTest::identity( | ||
StateCall::request((msg.clone().into(), LotusJson(shared_tipset.key().into()))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: looking at the verbosity of having to wrap things into LotusJson
and struggling with it during my own method impls - I think we could probably try and solve it somehow. I will just leave it here, perhaps we could eventually come up with a good way of solving this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist