use map as storage #2
Conversation
| const THRESHOLD_NUM: usize = 20000; | ||
| const THRESHOLD_DUM: usize = 30000; | ||
|
|
||
| pub struct Actor<S: LoadableState> { |
There was a problem hiding this comment.
State needs to be Serializable so it can be persisted and accessed through the actor boundary/FVM: https://github.com/adlrocha/hc-subnet-actor/blob/53627c07b21d2ae96831a94a33f274b5d53e1bc0/src/state.rs#L31
There was a problem hiding this comment.
Yeah, I had it placed on the struct. Once I have the hamt issue figured out, I'll just use the struct instead of trait. If I placed Serializable + Deserizable in the trait it would complicate stuff. So I just kept it simple for now.
| } | ||
|
|
||
| /// Map type to be used within actors. The underlying type is a HAMT. | ||
| pub type Map<'bs, BS, V> = Hamt<&'bs BS, V, BytesKey>; |
There was a problem hiding this comment.
It may be worth having a look at this: https://github.com/adlrocha/builtin-actors/blob/master/primitives/src/tcid/hamt.rs
It is a nice way of having Maps and types CIDs for safety. It also adds convenient methods to access map data.
| #[derive(Debug, Serialize, Deserialize)] | ||
| pub struct HamtState { | ||
| /// The list of node members in the registry | ||
| members: Cid, // HAMT<BytesKey from ActorID, NodeInfo> |
There was a problem hiding this comment.
As mentioned above, if you want to use typed CIDs (which I think it can be useful) see: https://github.com/adlrocha/builtin-actors/blob/master/primitives/src/tcid/mod.rs
| Ok(()) | ||
| } | ||
|
|
||
| fn report_checker(p: ReportPayload) -> Result<(), Error> { |
There was a problem hiding this comment.
I see that checkers are reported by ActorID. This means that the checker needs to do the conversion from peerID being reported to its ActorID off-chain, right?
There was a problem hiding this comment.
yeah, if that's possible. My assumption is which ever ActorID added the PeerID is responsible for ensure it's correct. Since there is really no way at the moment to tell if the actor id is actually related to the peer id.
src/hamt_state.rs
Outdated
| Ok(1) | ||
| } | ||
| Some(votes) => { | ||
| let t = self.vote_threshold(); |
There was a problem hiding this comment.
Shouldn't we record the vote before checking if it is withing threshold and removing the checker instead of waiting for the next vote to remove it?
(Maybe I haven't understood the code well, just to double-check)
There was a problem hiding this comment.
I updated this code already, there was a small issue when the voting duration is passed, new vote is not recorded in the new round. I have updated to recording already.
|
@cryptoAtwill, also for future PRs, it is useful to give a brief description of what we the PR is about for future reference (even if initially is just targeted to be reviewed internally by the team). We should always assume that folks are following the work we are doing, and it is useful to provide them with as much context as possible. |
|
BTW, note for the next iteration, it would be nice to add a few unit tests to cover at least the happy-path and see that we don't break anything in future versions |
|
@cryptoAtwill, we already have permissions to https://github.com/consensus-shipyard. Once this PR is merged I would suggest migrating the repo there (I am afraid you are the only one with the right combination of permissions to get it done). Thanks |
| @@ -3,8 +3,8 @@ use crate::traits::{LoadableState, UptimeCheckerActor}; | |||
| use crate::types::{InitParams, MultiAddr, NodeInfo, NodeInfoPayload, PeerID, ReportPayload}; | |||
| use crate::{ensure, Error}; | |||
|
|
|||
| const THRESHOLD_NUM: usize = 20000; | |||
| const THRESHOLD_DUM: usize = 30000; | |||
| const THRESHOLD_NUMERATOR: usize = 20000; | |||
There was a problem hiding this comment.
Why can't this be directly a float?
| const THRESHOLD_NUMERATOR: usize = 20000; | |
| const THRESHOLD = 2/3: |
There was a problem hiding this comment.
It's just for convenience I think. First if 2/3 is represented as a float, there are some lost precision. Representing with Numerator + Denominator can be flexible. At the same time, when using float, I need to cast total votes to f64. For this use case, I think should be ok, but usize -> u64, not sure there will be underflow or anything. And once multiplied, I need to round down/round up and convert back to usize to compare with count. If I just use NUMBERATOR + DENOMINATOR, I dont have to perform the casting or rounding.
There was a problem hiding this comment.
OK, I am convinced :) Can you add a few lines in the comments explaining this?? Thanks!
There was a problem hiding this comment.
LGTM! Feel free to merge. It would have been great if you had used the tcid to have typed cids instead of untyped ones as suggested here: https://github.com/cryptoAtwill/uptime-checker/pull/2/files#r973950197 but I leave to you the decision of fixing it here or not.
No description provided.