Setting up vbs integration #448
Conversation
| where | ||
| State: 'static + Send + Sync + ReadState, | ||
| <State as ReadState>::State: Send + Sync + AvailabilityDataSource<Types>, | ||
| Payload<Types>: QueryablePayload, | ||
| { | ||
| let mut api = load_api::<State, Error>( | ||
| let mut api = load_api::<State, Error, MAJOR_VERSION, MINOR_VERSION>( | ||
| options.api_path.as_ref(), | ||
| include_str!("../api/availability.toml"), | ||
| options.extensions.clone(), |
There was a problem hiding this comment.
We could change the with_version call below on line 160 to match, e.g format!("{MAJOR_VERSION}.{MINOR_VERSION}").parse().unwrap())
There was a problem hiding this comment.
Good call, but I think, instead, I'll add it as a default behavior in tide-disco and remove the .with_version calls on all the implementations. WDYT?
There was a problem hiding this comment.
Just a thought... this will lock the API version to the executable version, which was our intent. But we currently specify a semver::Version which takes (major, minor, patch) by default. I can update some things to allow us to accommodate semver, but we'll only have type enforcement on major and minor....
There was a problem hiding this comment.
Hmm. I actually don't necessarily thing the API and executable version should be locked. E.g. performance improvements to the executable, or features outside the scope of the versioned API, like rate limiting, API credits, etc.
Mayyybe we can have (major, minor) locked to the API version and use patch by convention for non-API updates. Or, we can just have separate versions, in which case I think the thing returned by the version endpoint should be the API version. We can make changes on tide disco if necessary to stop using semver and just use major,minor
src/availability.rs
Outdated
| @@ -503,7 +508,7 @@ mod test { | |||
|
|
|||
| /// Get the current ledger height and a list of non-empty leaf/block pairs. | |||
| async fn get_non_empty_blocks( | |||
| client: &Client<Error>, | |||
| client: &Client<Error, 0, 1>, | |||
There was a problem hiding this comment.
Is it possible to define like const LATEST_VERSION so we don't have to update every single one of these each time we do a version bump?
There was a problem hiding this comment.
Yeah, I think we can do one better, even... stay tuned, I'm still trying to wrangle all of the dependencies back to passing tests.
There was a problem hiding this comment.
So, it's currently still 0.1, but that's just because of HotShot's upgradability. I've got a fix for that. Note that the version is only specified outside of the sequencer itself in HS's upgradability, and in tests. I don't believe the actual version(s) used will matter for tests.
move47
force-pushed
the
adding_vbs_support
branch
from
0f2498e to
fd5c425
Compare
nyospe
force-pushed
the
adding_vbs_support
branch
from
2971fc5 to
6a4ba00
Compare
Resolves EspressoSystems/versioned-binary-serialization#25