-
Notifications
You must be signed in to change notification settings - Fork 18
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
Remove soroban-spec-tools #80
Conversation
}) => Ok( | ||
contract::Spec::new(&self.get_remote_wasm_from_hash(hash).await?) | ||
.map_err(Error::CouldNotParseContractSpec)? | ||
.spec, | ||
), | ||
}) => Ok(soroban_spec::read::from_wasm( | ||
&self.get_remote_wasm_from_hash(hash).await?, | ||
)?), |
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.
This is the main part of the change. The soroban-spec
crate already provides the same functionality that the soroban-spec-tools
crate wraps and adds additional CLI related functionality to. There's no need for the RPC to use the soroban-spec-tools
crate for this as it doesn't need the CLI related functionality.
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.
Nice !!
The Go linter check is failing, but this PR doesn't touch the Go code. |
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.
Ignore the golangcli failures for now (I am looking into it)
Overall looks good !!
}) => Ok( | ||
contract::Spec::new(&self.get_remote_wasm_from_hash(hash).await?) | ||
.map_err(Error::CouldNotParseContractSpec)? | ||
.spec, | ||
), | ||
}) => Ok(soroban_spec::read::from_wasm( | ||
&self.get_remote_wasm_from_hash(hash).await?, | ||
)?), |
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.
Nice !!
What
Remove soroban-spec-tools.
Why
The soroban-spec-tools crate primarily exists to support the CLI. It contains logic that originally existed in the CLI and was separated into a library.
The soroban-rpc is using one function of the crate, but actually that function is just a call through to the soroban-spec crate. There's no need for the soroban-rpc to depend on the other logic in the soroban-spec-tools crate and it is inconvenient that it does.
It's inconvenient because the soroban-spec-tools crate is closely coupled with the soroban-cli, and changes to both of those together are more likely than changes to the rpc and it.
This came up when I tried to change the output of the soroban-cli, and to change the output of the cli I need to change the soroban-spec-tools crate.
This PR should be accompanied by another PR that moves the soroban-spec-tools crate back to the soroban-tools/cli repo.
Related: