-
Notifications
You must be signed in to change notification settings - Fork 490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tools: block-generator applications. Part 1: create #5450
Conversation
"github.com/algorand/go-algorand/protocol" | ||
) | ||
|
||
func (g *generator) makeTxnHeader(sender basics.Address, round, intra uint64) transactions.Header { | ||
// ---- header / boilerplate ---- | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this diff is confusing. I didn't actually modify makeTxnHeader
except for renaming the transactions
import (→ txn
). But I did introduce makeTestTxn
which is very similar to makeTxnHeader
and confused the differ.
@@ -20,15 +20,34 @@ import ( | |||
"encoding/binary" | |||
|
|||
"github.com/algorand/go-algorand/data/basics" | |||
"github.com/algorand/go-algorand/data/transactions" | |||
txn "github.com/algorand/go-algorand/data/transactions" | |||
"github.com/algorand/go-algorand/data/txntest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
txntest.Txn
is the modern convenience struct for testing transactions
return signTxn(txn), transactions.ApplyData{}, nil | ||
} | ||
|
||
func (g *generator) generateAppCallInternal(txType TxTypeID, round, intra, hintIndex uint64, hintApp *appData) (TxTypeID, transactions.Transaction, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hintIndex not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep this param until my follow up PR is ready for review, at which point I commit to removing if still unused.
Let's not resolve this discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's removed in #5478
numApps := uint64(len(g.apps[kind])) | ||
if numApps == 0 { | ||
actualAppTx = appTxTypeCreate | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this block necessary? you've assert at line 799 that appTx must be appTxTypeCreate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another one I'd like to keep till the next PR is ready, and so let's not resolve this discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic has been fixed in #5478
note := make([]byte, 8) | ||
binary.LittleEndian.PutUint64(note, uint64(g.txnCounter+intra)) | ||
|
||
return transactions.Header{ | ||
return txntest.Txn{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
} | ||
|
||
// makeTestTxn creates and populates the flat txntest.Txn structure with the given values. | ||
func (g *generator) makeTestTxn(sender basics.Address, round, intra uint64) txntest.Txn { | ||
note := make([]byte, 8) | ||
binary.LittleEndian.PutUint64(note, uint64(g.txnCounter+intra)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: uint64 casting is not needed; same for makeTxnHeader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ad = *g.pendingApps[appKindSwap][0] | ||
holding = *ad.holdings[0] | ||
require.Equal(t, holding, *ad.holders[0]) | ||
require.Equal(t, uint64(1001), holding.appIndex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised the appID is still 1001. I expected to have incremented to 1002.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because I'm not passing the new intra
into generateAppCallInternal()
. I'm much more explicit about this in the next PR. I propose keeping this thread open and adding it as a TODO to the follow up.
Co-authored-by: shiqizng <80276844+shiqizng@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit. LGTM
Co-authored-by: Will Winder <wwinder.unh@gmail.com>
Summary
Handle application creation in the block generator including local storage.
TODO in a follow up PR
app_boxes
Blocked by:
tools: bugfix block-generator to handle conduit's Init block requests #5449tools: prepare block-generator for configuring apps #5443Test Plan
Some unit tests are added. However, most of the testing is done locally. You can try at home as follows:
NOTE: I mixed/matched some outputs from various runs so the numbers below are inconsistent