Initial Refactor to Remove the Worker Abstraction #109
Conversation
chadlagore
force-pushed
the
106-refactor-work-structure
branch
from
8df38ac
to
01f08ce
Compare
| func TestRequestHandlerNewBlockOK(t *testing.T) { | ||
| initializeChain() | ||
| a := App{PeerStore: peer.NewPeerStore("127.0.0.1:8000")} | ||
| // TODO: Enable once block request by hash implemented. |
There was a problem hiding this comment.
@david-julien is working on something to fix this test
| CurrentUser *User | ||
| PeerStore *peer.PeerStore | ||
| Chain *blockchain.BlockChain | ||
| Pool *pool.Pool |
app/app.go
Outdated
| } | ||
|
|
||
| // BlockWorkQueue is a queue of blocks to process. | ||
| var blockQueue = make(chan *blockchain.Block, blockQueueSize) |
There was a problem hiding this comment.
I would wrap these all up like this:
var (
blockQueue = make(chan *blockchain.Block, blockQueueSize)
...
)
app/app.go
Outdated
| @@ -110,7 +121,7 @@ func Run(cfg conf.Config) { | |||
| } | |||
| log.Info("Redirecting logs to logfile") | |||
| log.SetOutput(logFile) | |||
| go RunConsole(a.PeerStore) | |||
| go RunConsole(a.PeerStore, &a) | |||
There was a problem hiding this comment.
Might as well change RunConsole to only take the app instance as an argument since we can access the PeerStore from a.
app/app.go
Outdated
| // TODO: If not, request chain from peers. | ||
| // HandleTransaction handles new instance of TransactionWork. | ||
| func (a *App) HandleTransaction(txn *blockchain.Transaction) { | ||
| validTransaction := a.Pool.Set(txn, a.Chain) |
There was a problem hiding this comment.
Can we rename Set to Insert or Add? I feel like Set doesn't really make sense give what it actually does.
There was a problem hiding this comment.
Sure! Push seems good to me. It goes on the back of the queue.
| }) | ||
| shell.AddCmd(&ishell.Cmd{ | ||
| Name: "peers", | ||
| Help: "show the peers this host is connected to", | ||
| Func: peers, | ||
| Func: func(ctx *ishell.Context) { |
There was a problem hiding this comment.
Why wrap function calls in new anonymous functions? Couldn't you, for example, make peers take ctx and then just do the stuff peers currently does? That would make the code shorter and more readable.
There was a problem hiding this comment.
I see now that you're doing it because you only have access to app from Run(), but that need not be the case in my opinion.
There was a problem hiding this comment.
The original situation was making app and peerStore global, which seemed less than ideal to me.
pool/pool.go
Outdated
| @@ -64,11 +71,13 @@ func getIndex(a []*PooledTransaction, target time.Time, low, high int) int { | |||
| // Set inserts a transaction into the pool, returning | |||
| // true if the Transaction was inserted (was valid). | |||
| func (p *Pool) Set(t *blockchain.Transaction, bc *blockchain.BlockChain) bool { | |||
| if ok, _ := bc.ValidTransaction(t); ok { | |||
| if ok, err := bc.ValidTransaction(t); ok { | |||
| p.set(t) | |||
There was a problem hiding this comment.
Will this succeed (ie add the transaction to the pool) if the transaction is valid wrt the blockchain but is not valid wrt some transaction already in the pool? ie. the input already exists in the pool. Does p.set handle that?
There was a problem hiding this comment.
Since the transactions are mapped by input hash, this type of situation currently would overwrite the existing transaction in the pool. Thats better than letting a double spend, but its not great. I'll work on it 👍
| PeerStore: peer.NewPeerStore(addr), | ||
| PeerStore: peer.NewPeerStore(addr), | ||
| CurrentUser: getCurrentUser(), | ||
| Chain: getLocalChain(), |
There was a problem hiding this comment.
This is not great. In the case when the chain is not yet local, we don't exactly want to kick off a torrenting at this exact moment. We might want the chain download to be voluntary. A CLI request 🤔
chadlagore
changed the title
Status: Ready to merge.
Related Issue
106
Description
Todos
General: