Sequencer builder

[Mirko-von-Leipzig]

PR with my attempt at making a request builder for the sequencer queries. It uses the typestate pattern.

Current usage:

    let block: Starknetlock = self
        .request()    
        .feeder_gateway()
        .get_block()
        .at_block(block_number)
        .with_retry(Retry::Enabled)
        .get()
        .await
        .unwrap();

Still todo:

• docs
• move parsing to builder (?)
• move retry to builder (?)

Also somewhat strangely, the tests for the transaction write's fail when executed locally, but pass when using the sequencer. I suspect it has something to do with the test setup; but I haven't figured out what / how to fix it.

This is due to the warp test server requiring query params be set even when there aren't any. Our previous request implementation always set these params to [] whereas this new one never sets them if there aren't any (None).

I've also collapsed the block_by_hash and block_by_number into a single call: block(block_id) where BlockId can represent Hash, Number, Latest and Pending.

[Mirko-von-Leipzig]

I've left a lot of the one-liner functions undocumented as the function name should be enough in those cases.

It would also have been nicer to link to the actual Request<stage> impls directly, but unfortunately rustdoc does not support that.

[Mirko-von-Leipzig]

I wanted the following:

Suggested change

	async fn block(&self, block: BlockId) -> Result<reply::Block, SequencerError>;
	async fn block<B: Into<BlockId>>(&self, block: B)-> Result<reply::Block, SequencerError>;

but unfortunately this does not work with the mock library. Mostly just means that callers must append .into() themselves on some calls. Might revisit this if / when we remove mocks for channels.

[koivunej]

It is better probably overall but this probably would had removed the object safety from the method, which is why it cannot be used. In general there has recent-ishly been discussion on the amount of bloat this pattern creates and what kind of countermeasures it needs, for example some stdlib functions are constructed with an generic layer on top of unsized reference accepting arg: https://doc.rust-lang.org/stable/src/std/fs.rs.html#228-236

[Mirko-von-Leipzig]

Uncertain why that test times out. It happens locally as well, but only if using the local sequencer mock.

This was due to not disabling retries under test cfg. This has now been fixed.

[koivunej]

Shouldn't there be a lifetime on the return value?

[Mirko-von-Leipzig]

Appears to be ellided? Or rather, it hasn't forced me to add it 🤔

[koivunej]

Need to add the warning then on these. I guess too many write them out by default.

Or not ... There's 41 in this repo of #[warn(rust_2018_idioms)]. I would prefer not to add to that. As a deny this looks like:

error: hidden lifetime parameters in types are deprecated
 --> crates/pathfinder/src/storage/schema/revision_0011.rs:8:37
  |
8 | pub(crate) fn migrate(transaction: &Transaction) -> anyhow::Result<PostMigrationAction> {
  |                                     ^^^^^^^^^^^ expected named lifetime parameter
  |
help: consider using the `'_` lifetime
  |
8 | pub(crate) fn migrate(transaction: &Transaction<'_>) -> anyhow::Result<PostMigrationAction> {
  |                                     ~~~~~~~~~~~~~~~

It is most likely cargo-fixable. I don't know why this isn't enabled automatically.

[Mirko-von-Leipzig]

Tracking issue for warning for rust_2018_idioms by default indicates they were being conservative.

Personally, I didn't even know this was a thing :).

[Mirko-von-Leipzig]

I'll create a new PR which fixes the whole codebase and adds the check to ci.

[CHr15F0x]

LGTM! I really like the way queries are built now 🎉 .
Would be nice to disable those retries in tests.