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.

Sign up for GitHub

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

Jump to bottom

[deps] update rpc deps #2044

Merged
merged 4 commits into from
Jul 2, 2024
Merged

[deps] update rpc deps #2044

merged 4 commits into from
Jul 2, 2024

Conversation

vegetabledogdog
Copy link
Contributor

@vegetabledogdog vegetabledogdog commented Jul 1, 2024 •
edited
Loading

@vercel Vercel
Copy link

vercel bot commented Jul 1, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rooch-portal ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 2, 2024 7:22am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
rooch ⬜️ Ignored (Inspect) Visit Preview Jul 2, 2024 7:22am

@jolestar
Copy link
Contributor

jolestar commented Jul 1, 2024

@baichuan3 help to review.

baichuan3
baichuan3 reviewed Jul 2, 2024
View reviewed changes
crates/rooch-rpc-client/src/lib.rs
@@ -50,8 +44,7 @@ impl ClientBuilder {

let http_client = Arc::new(
HttpClientBuilder::default()
.max_request_body_size(2 << 30)
.max_concurrent_requests(self.max_concurrent_requests)
Copy link
Collaborator

Choose a reason for hiding this comment

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

新版本的 jsonrpsee 移除了max_concurrent_requests支持？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

是的
参考这个：paritytech/jsonrpsee#1377

baichuan3
baichuan3 reviewed Jul 2, 2024
View reviewed changes
crates/rooch-rpc-server/src/server/btc_server.rs Outdated
let global_state_filter =
UTXOFilterView::into_global_state_filter(filter, resolve_address)?;
let global_state_filter = UTXOFilterView::into_global_state_filter(filter, resolve_address)
.map_err(|e| err_obj(e.to_string()))?;
Copy link
Collaborator

@baichuan3 baichuan3 Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

还有一种思路， 现在 RPC 是直接使用 Jsonrpcsee里的RpcResult和Error，可以考虑封装Rooch 自己的RpcResult和Error，将 Jsonrpcsee 的 Error 自动转换成 RpcResult里的 Error，这样的好处就是，不需要在每一个 RPC API实现里，显示的调用map_err(|e| err_obj(e.to_string()))?方法，代码会更简洁。

可参考 sui 的实现 https://github.com/MystenLabs/sui/blob/a7bc3198073f29d285ae36acf77ceda8032a548d/crates/sui-json-rpc/src/error.rs#L35-L36

Copy link
Contributor Author

Choose a reason for hiding this comment

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

可以，我试试

baichuan3
baichuan3 reviewed Jul 2, 2024
View reviewed changes
crates/rooch-rpc-server/src/lib.rs
@@ -337,8 +336,7 @@ pub async fn run_start_server(opt: RoochOpt, server_opt: ServerOpt) -> Result<Se

// Build server
let server = ServerBuilder::default()
.set_logger(RpcLogger)
Copy link
Collaborator

Choose a reason for hiding this comment

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

rpc logger 为啥也去掉了？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jsonrpcseed的logger转移到了middware，现在用的middware是tower库的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/paritytech/jsonrpsee/blob/9ebc4c16353b828e254ad7679947383a79984dbf/CHANGELOG.md?plain=1#L383

Copy link
Contributor Author

Choose a reason for hiding this comment

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

等我改一下

@baichuan3
Copy link
Collaborator

jsonrpcsee 0.16x 版本有个 bug，在构造http builder时，需要强制传入端口， 当时代码里暂时用了workaround 的办法，https://dev-seed.rooch.network:443/

jsonrpcsee在 0.17版本中修复了该 bug，可以验证下升级后client_config.rs里构造 http builder去掉端口号后是否正常， ref #869。

@vegetabledogdog
Copy link
Contributor Author

jsonrpcsee 0.16x 版本有个 bug，在构造http builder时，需要强制传入端口， 当时代码里暂时用了workaround 的办法，https://dev-seed.rooch.network:443/

jsonrpcsee在 0.17版本中修复了该 bug，可以验证下升级后client_config.rs里构造 http builder去掉端口号后是否正常， ref #869。

去掉端口，工作正常

baichuan3
baichuan3 approved these changes Jul 2, 2024
View reviewed changes
Copy link
Collaborator

@baichuan3 baichuan3 left a comment

Choose a reason for hiding this comment

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

LGTM

@jolestar jolestar merged commit 4e5deac into rooch-network:main Jul 2, 2024
6 of 7 checks passed
@vegetabledogdog vegetabledogdog deleted the deps branch July 9, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baichuan3 baichuan3 baichuan3 approved these changes

@wow-sven wow-sven Awaiting requested review from wow-sven wow-sven is a code owner

@pause125 pause125 Awaiting requested review from pause125 pause125 is a code owner

@jolestar jolestar Awaiting requested review from jolestar jolestar is a code owner

@popcnt1 popcnt1 Awaiting requested review from popcnt1 popcnt1 is a code owner

@steelgeek091 steelgeek091 Awaiting requested review from steelgeek091 steelgeek091 is a code owner

Assignees

@baichuan3 baichuan3

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[deps] Upgrade jsonrpsee to 0.20.x
3 participants
@vegetabledogdog @jolestar @baichuan3