-
Notifications
You must be signed in to change notification settings - Fork 470
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 txpool namespace support #382
Conversation
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.
Looks really good for a start, thanks! I'd like to see a bit more strict typing and some documentation pointers.
src/types/txpool.rs
Outdated
#[derive(Debug, Default, Clone, PartialEq, Deserialize, Serialize)] | ||
pub struct TxpoolContentInfo { | ||
/// pending tx | ||
pub pending: HashMap<String, HashMap<String, Transaction>>, |
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.
Perhaps better to use BTreeMap
here to have the keys deterministically ordered? With HashMap
the results would be a bit random. Also we should be using more strict types than String
. I believe it's transaction hash and the nonce, right?
pub pending: HashMap<String, HashMap<String, Transaction>>, | |
pub pending: BTreeMap<H256, BTreeMap<U256, Transaction>>, |
src/types/txpool.rs
Outdated
/// pending tx | ||
pub pending: HashMap<String, HashMap<String, Transaction>>, | ||
/// queued tx | ||
pub queued: HashMap<String, HashMap<String, Transaction>>, |
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.
pub queued: HashMap<String, HashMap<String, Transaction>>, | |
pub queued: BTreeMap<H256, BTreeMap<U256, Transaction>>, |
src/types/txpool.rs
Outdated
#[derive(Debug, Default, Clone, PartialEq, Deserialize, Serialize)] | ||
pub struct TxpoolInspectInfo { | ||
/// pending tx | ||
pub pending: HashMap<String, HashMap<String, String>>, |
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.
pub pending: HashMap<String, HashMap<String, String>>, | |
pub pending: BTreeMap<H256, BTreeMap<U256, String>>, |
src/types/txpool.rs
Outdated
/// pending tx | ||
pub pending: HashMap<String, HashMap<String, String>>, | ||
/// queued tx | ||
pub queued: HashMap<String, HashMap<String, String>>, |
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.
pub queued: HashMap<String, HashMap<String, String>>, | |
pub queued: BTreeMap<H256, BTreeMap<U256, String>>, |
pub queued: HashMap<String, HashMap<String, Transaction>>, | ||
} | ||
|
||
/// Transaction Pool Inspect Info |
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.
Can we expand on the docs or at least point to documentation of these structs? I'd be more interested in the String
format of the transaction inspection info - i.e. what things can be found there and can we actually parse it?
src/types/txpool.rs
Outdated
"pending":"0x23", | ||
"queued":"0x20" | ||
}"#; | ||
let _: TxpoolStatus = serde_json::from_str(txpool_status_str).unwrap(); |
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.
Re-serialization test would be cool, after switching to BTreeMap
you will get consistent results.
@tomusdrw Thank you for the review comments. I made changes as follows.
Note that I keep using |
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.
looks awesome, thanks!
This is a PR for the issue #381. Please review and merge it if you like.