Skip to content
This repository was archived by the owner on Aug 30, 2022. It is now read-only.

Conversation

@Robert-Steiner
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Dec 17, 2020

Codecov Report

Merging #636 (e54b430) into master (dbaa0e5) will decrease coverage by 0.51%.
The diff coverage is 37.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #636      +/-   ##
==========================================
- Coverage   61.51%   61.00%   -0.52%     
==========================================
  Files         102      102              
  Lines        4563     4559       -4     
==========================================
- Hits         2807     2781      -26     
- Misses       1756     1778      +22     
Impacted Files Coverage Δ
rust/xaynet-server/src/settings/mod.rs 90.62% <0.00%> (-1.57%) ⬇️
rust/xaynet-server/src/settings/s3.rs 91.30% <ø> (ø)
rust/xaynet-server/src/state_machine/mod.rs 75.00% <ø> (ø)
...ver/src/storage/coordinator_storage/redis/impls.rs 72.72% <0.00%> (ø)
...erver/src/storage/coordinator_storage/redis/mod.rs 71.20% <ø> (ø)
rust/xaynet-server/src/storage/traits.rs 66.66% <ø> (ø)
...ynet-server/src/storage/trust_anchor/iota/utils.rs 0.00% <0.00%> (ø)
...net-server/src/storage/trust_anchor/iota/client.rs 2.98% <2.98%> (ø)
rust/xaynet-server/src/storage/store.rs 80.00% <72.72%> (ø)
...t/xaynet-server/src/state_machine/phases/unmask.rs 58.33% <75.00%> (ø)
... and 50 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dbaa0e5...8feec80. Read the comment docs.

@Robert-Steiner
Copy link
Member Author

/deploy COORDINATOR_FEATURES=metrics,iota

@Robert-Steiner Robert-Steiner force-pushed the trust_anchor branch 2 times, most recently from 1ef0cca to 4c5beb5 Compare December 17, 2020 11:20
@Robert-Steiner
Copy link
Member Author

/deploy COORDINATOR_FEATURES=metrics,iota

Copy link
Contributor

@wilk10 wilk10 left a comment

Choose a reason for hiding this comment

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

Thanks a lot! LGTM, but perhaps seek the approval of someone else too?

}

#[async_trait]
pub trait Storage: CoordinatorStorage + ModelStorage + TrustAnchor {
Copy link
Contributor

Choose a reason for hiding this comment

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

Very neat :) Funny how such an innocent looking trait leads to such improvement through the entire state machine. Why is the is_ready needed though? CoordinatorStorage, ModelStorage and TrustAnchor all have an is_ready method already.

Copy link
Member Author

Choose a reason for hiding this comment

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

@little-dude 🎉

Very neat :) Funny how such an innocent looking trait leads to such improvement through the entire state machine.

Yeah I agree😊. It is awesome to see that it work this way without dynamic dispatch.

The is_ready method combines the is_ready methods of CoordinatorStorage, ModelStorage and TrustAnchor and is called in the state machine error phase.

Copy link
Contributor

Choose a reason for hiding this comment

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

@little-dude !! are you dynamically dispatching meals to people doorsteps already?

Copy link
Contributor

@finiteprods finiteprods left a comment

Choose a reason for hiding this comment

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

looks great. I don't have much domain knowledge around the iota parts, so can't say much there. perhaps more docstrings there (particularly for pub functions) would be helpful, but you don't need to do it straight away. Also I wasn't able to see your lovely module documentation when i did cargo doc, maybe just a problem with my setup, or was this intentional? I agree with @little-dude 's comment that the changes to the state machine make it simpler and more extensible, which is great.

/// This contains the state-dependent `private` state and the state-independent `shared` state
/// which is shared across state transitions.
pub struct PhaseState<S, C, M>
pub struct PhaseState<State, Store>
Copy link
Contributor

Choose a reason for hiding this comment

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

personally for consistency i would say <S, T> and add a comment on what they represent, but I'm ok with this too if you prefer it.

/// XAYNET_TRUST_ANCHOR__IOTA__NETWORK=Devnet
/// ```
pub network: Network,
/// The seed of the author. Allowed characters are: `A-Z` and `9`.
Copy link
Contributor

Choose a reason for hiding this comment

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

A-Z and just 9? that's quirky...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants