feat: B2 pikachu: integration of KVS and Bitcoin testnet and some refactoring #154
Conversation
… up to threshold)
Routing messages through hierarchy
Bumps [acorn](https://github.com/acornjs/acorn) from 5.7.3 to 5.7.4. - [Release notes](https://github.com/acornjs/acorn/releases) - [Commits](acornjs/acorn@5.7.3...5.7.4) --- updated-dependencies: - dependency-name: acorn dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>
|
@sa8, do you mind pointing us to the right files to review (or pay more attention to)? There are 632 files changed 🙈 I guess this is as a result of your rebase with eudico (main) and the fact that your destination branch is not rebased. Thanks! |
|
ha yes of course 😅. |
There was a problem hiding this comment.
hey, @sa8. I did a first round of comments to the checkpointing package. I hope they are useful. If there is anything else you want me to have a deeper look at let me know.
As we are only merging this to your WIP branch, feel free to address the ones you feel are straightforward and merge whenever you are ready. Let's worry about cleaning the code and addressing what is pending once everything works and we are ready to merge to the main branch (and we are able to review everything as a whole).
| "encoding/hex" | ||
| "fmt" | ||
|
|
||
| // "github.com/filecoin-project/go-address" |
There was a problem hiding this comment.
Just a note for the future: we'll need to clean dangling comments and debugging prints throughout the code before merging with the main branch.
Also, it may be worth adding a note here to remind ourselves to merge your kvs with hierarchical consensus' content resolution (they are quite similar and we may be able to generalize it)
| // Message type being propagated | ||
| Type MsgType | ||
| // Cid of the content | ||
| Cid string |
There was a problem hiding this comment.
Why string instead of cid.Cid?
There was a problem hiding this comment.
Oh, I see. You are not actually using a Cid but a raw hash as the key for content (see comments below).
| // Checkpoint schema.Checkpoint | ||
|
|
||
| //for checkpointing, we use []byte | ||
| Content MsgData |
There was a problem hiding this comment.
MsgData is of type []byte, why not using Content []byte directly?
There was a problem hiding this comment.
hmm I think at the time I did it because I had issue with my cbor but now that you mention it it does looks unnecessary 🤔
|
|
||
| c.participants = participants | ||
| //TODO: change this, import the wallet automatically |
| //start by getting the balance in our wallet (only if sendall is true, i.e. we send all the amount) | ||
| var value float64 | ||
| if sendall { | ||
| payload1 := "{\"jsonrpc\": \"1.0\", \"id\":\"wow\", \"method\": \"getbalances\", \"params\": []}" |
There was a problem hiding this comment.
I made the comment somewhere else. Using structs and json.Marshal for this would make the code way more readable (unless there is a strong reason to do it this way).
Related Issues
#102 #103 #117 #55
Proposed Changes
Additional Info
More information about the projects can be found here: consensus-shipyard/consensuslab#5
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>fix: mempool: Introduce a cache for valid signaturesPR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, testarea: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps