Skip to content
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

Chore: Use exp/slices and exp/maps to simplify some code #5479

Merged
merged 13 commits into from
Jun 22, 2023
23 changes: 5 additions & 18 deletions agreement/agreementtest/simulate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (

"github.com/algorand/go-deadlock"
"github.com/stretchr/testify/require"
"golang.org/x/exp/maps"

"github.com/algorand/go-algorand/agreement"
"github.com/algorand/go-algorand/config"
Expand Down Expand Up @@ -124,24 +125,10 @@ func makeTestLedger(state map[basics.Address]basics.AccountData) agreement.Ledge
func (l *testLedger) copy() *testLedger {
dup := new(testLedger)

dup.entries = make(map[basics.Round]bookkeeping.Block)
dup.certs = make(map[basics.Round]agreement.Certificate)
dup.state = make(map[basics.Address]basics.AccountData)
dup.notifications = make(map[basics.Round]signal)

for k, v := range l.entries {
dup.entries[k] = v
}
for k, v := range l.certs {
dup.certs[k] = v
}
for k, v := range l.state {
dup.state[k] = v
}
for k, v := range dup.notifications {
// note that old opened channels will now fire when these are closed
dup.notifications[k] = v
}
dup.entries = maps.Clone(l.entries)
dup.certs = maps.Clone(l.certs)
dup.state = maps.Clone(l.state)
dup.notifications = maps.Clone(l.notifications) // old opened channels will now fire when these are closed
dup.nextRound = l.nextRound

return dup
Expand Down
5 changes: 2 additions & 3 deletions agreement/autopsy.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
"github.com/algorand/go-algorand/data/basics"
"github.com/algorand/go-algorand/logging"
"github.com/algorand/go-algorand/protocol"
"golang.org/x/exp/slices"
)

// An Autopsy is a trace of the ordered input events and output
Expand Down Expand Up @@ -102,9 +103,7 @@

// makeMultiCloser returns a Closer that closes all the given closers.
func makeMultiCloser(closers ...io.Closer) io.Closer {
r := make([]io.Closer, len(closers))
copy(r, closers)
return &multiCloser{r}
return &multiCloser{slices.Clone(closers)}

Check warning on line 106 in agreement/autopsy.go

View check run for this annotation

Codecov / codecov/patch

agreement/autopsy.go#L106

Added line #L106 was not covered by tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not even sure this needs the clone. When called as varargs, the slice is surely unshared.

}

type autopsyTrace struct {
Expand Down
16 changes: 4 additions & 12 deletions agreement/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

"github.com/algorand/go-deadlock"
"github.com/stretchr/testify/require"
"golang.org/x/exp/maps"

"github.com/algorand/go-algorand/config"
"github.com/algorand/go-algorand/crypto"
Expand Down Expand Up @@ -207,10 +208,7 @@ func makeTestLedger(state map[basics.Address]basics.AccountData) Ledger {
l.certs = make(map[basics.Round]Certificate)
l.nextRound = 1

l.state = make(map[basics.Address]basics.AccountData)
for k, v := range state {
l.state[k] = v
}
l.state = maps.Clone(state)

l.notifications = make(map[basics.Round]signal)

Expand All @@ -226,10 +224,7 @@ func makeTestLedgerWithConsensusVersion(state map[basics.Address]basics.AccountD
l.certs = make(map[basics.Round]Certificate)
l.nextRound = 1

l.state = make(map[basics.Address]basics.AccountData)
for k, v := range state {
l.state[k] = v
}
l.state = maps.Clone(state)

l.notifications = make(map[basics.Round]signal)

Expand All @@ -245,10 +240,7 @@ func makeTestLedgerMaxBlocks(state map[basics.Address]basics.AccountData, maxNum

l.maxNumBlocks = maxNumBlocks

l.state = make(map[basics.Address]basics.AccountData)
for k, v := range state {
l.state[k] = v
}
l.state = maps.Clone(state)

l.notifications = make(map[basics.Round]signal)

Expand Down
6 changes: 2 additions & 4 deletions cmd/algokey/keyreg.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"strings"

"github.com/spf13/cobra"
"golang.org/x/exp/maps"

"github.com/algorand/go-algorand/crypto"
"github.com/algorand/go-algorand/data/account"
Expand Down Expand Up @@ -94,10 +95,7 @@ func init() {
"betanet": mustConvertB64ToDigest("mFgazF+2uRS1tMiL9dsj01hJGySEmPN28B/TjjvpVW0="),
"devnet": mustConvertB64ToDigest("sC3P7e2SdbqKJK0tbiCdK9tdSpbe6XeCGKdoNzmlj0E="),
}
validNetworkList = make([]string, 0, len(validNetworks))
for k := range validNetworks {
validNetworkList = append(validNetworkList, k)
}
validNetworkList = maps.Keys(validNetworks)
}

func mustConvertB64ToDigest(b64 string) (digest crypto.Digest) {
Expand Down
13 changes: 5 additions & 8 deletions cmd/goal/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
"time"

"github.com/spf13/cobra"
"golang.org/x/exp/slices"

"github.com/algorand/go-algorand/cmd/util/datadir"
"github.com/algorand/go-algorand/config"
Expand Down Expand Up @@ -557,35 +558,31 @@
func printAccountInfo(client libgoal.Client, address string, onlyShowAssetIds bool, account model.Account) bool {
var createdAssets []model.Asset
if account.CreatedAssets != nil {
createdAssets = make([]model.Asset, len(*account.CreatedAssets))
copy(createdAssets, *account.CreatedAssets)
createdAssets = slices.Clone(*account.CreatedAssets)

Check warning on line 561 in cmd/goal/account.go

View check run for this annotation

Codecov / codecov/patch

cmd/goal/account.go#L561

Added line #L561 was not covered by tests
sort.Slice(createdAssets, func(i, j int) bool {
return createdAssets[i].Index < createdAssets[j].Index
})
}

var heldAssets []model.AssetHolding
if account.Assets != nil {
heldAssets = make([]model.AssetHolding, len(*account.Assets))
copy(heldAssets, *account.Assets)
heldAssets = slices.Clone(*account.Assets)

Check warning on line 569 in cmd/goal/account.go

View check run for this annotation

Codecov / codecov/patch

cmd/goal/account.go#L569

Added line #L569 was not covered by tests
sort.Slice(heldAssets, func(i, j int) bool {
return heldAssets[i].AssetID < heldAssets[j].AssetID
})
}

var createdApps []model.Application
if account.CreatedApps != nil {
createdApps = make([]model.Application, len(*account.CreatedApps))
copy(createdApps, *account.CreatedApps)
createdApps = slices.Clone(*account.CreatedApps)

Check warning on line 577 in cmd/goal/account.go

View check run for this annotation

Codecov / codecov/patch

cmd/goal/account.go#L577

Added line #L577 was not covered by tests
sort.Slice(createdApps, func(i, j int) bool {
return createdApps[i].Id < createdApps[j].Id
})
}

var optedInApps []model.ApplicationLocalState
if account.AppsLocalState != nil {
optedInApps = make([]model.ApplicationLocalState, len(*account.AppsLocalState))
copy(optedInApps, *account.AppsLocalState)
optedInApps = slices.Clone(*account.AppsLocalState)

Check warning on line 585 in cmd/goal/account.go

View check run for this annotation

Codecov / codecov/patch

cmd/goal/account.go#L585

Added line #L585 was not covered by tests
sort.Slice(optedInApps, func(i, j int) bool {
return optedInApps[i].Id < optedInApps[j].Id
})
Expand Down
2 changes: 1 addition & 1 deletion cmd/opdoc/opdoc.go
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ func main() {
AVMType: t.AVMType.String(),
})
}
sort.Slice(named, func(i, j int) bool { return strings.Compare(named[i].Name, named[j].Name) > 0 })
sort.Slice(named, func(i, j int) bool { return named[i].Name > named[j].Name })

namedStackTypes := create("named_stack_types.md")
namedStackTypesMarkdown(namedStackTypes, named)
Expand Down
4 changes: 2 additions & 2 deletions cmd/tealdbg/local.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"github.com/algorand/go-algorand/data/transactions/logic"
"github.com/algorand/go-algorand/ledger/apply"
"github.com/algorand/go-algorand/protocol"
"golang.org/x/exp/slices"
)

func protoFromString(protoString string) (name string, proto config.ConsensusParams, err error) {
Expand Down Expand Up @@ -190,8 +191,7 @@
b.locals[addr][aid] = tkv.Clone()
}
}
b.logs = make([]string, len(a.logs))
copy(b.logs, a.logs)
b.logs = slices.Clone(a.logs)

Check warning on line 194 in cmd/tealdbg/local.go

View check run for this annotation

Codecov / codecov/patch

cmd/tealdbg/local.go#L194

Added line #L194 was not covered by tests
b.innerTxns = cloneInners(a.innerTxns)
return
}
Expand Down
3 changes: 2 additions & 1 deletion crypto/merklearray/merkle.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"sort"

"github.com/algorand/go-algorand/crypto"
"golang.org/x/exp/slices"
)

const (
Expand Down Expand Up @@ -223,7 +224,7 @@ func (tree *Tree) Prove(idxs []uint64) (*Proof, error) {
idxs = VcIdxs
}

sort.Slice(idxs, func(i, j int) bool { return idxs[i] < idxs[j] })
slices.Sort(idxs)

return tree.createProof(idxs)
}
Expand Down
11 changes: 5 additions & 6 deletions crypto/merkletrie/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ import (
"encoding/binary"
"errors"
"fmt"
"sort"

"golang.org/x/exp/maps"
"golang.org/x/exp/slices"
)

// storedNodeIdentifier is the "equivalent" of a node-ptr, but oriented around persisting the
Expand Down Expand Up @@ -446,11 +448,8 @@ func (mtc *merkleTrieCache) reallocatePendingPages(stats *CommitStats) (pagesToC
}

// create a sorted list of created pages
sortedCreatedPages := make([]uint64, 0, len(createdPages))
for page := range createdPages {
sortedCreatedPages = append(sortedCreatedPages, page)
}
sort.SliceStable(sortedCreatedPages, func(i, j int) bool { return sortedCreatedPages[i] < sortedCreatedPages[j] })
sortedCreatedPages := maps.Keys(createdPages)
slices.Sort(sortedCreatedPages)
jasonpaulos marked this conversation as resolved.
Show resolved Hide resolved

mtc.reallocatedPages = make(map[uint64]map[storedNodeIdentifier]*node)

Expand Down
6 changes: 3 additions & 3 deletions crypto/merkletrie/committer.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package merkletrie

import "golang.org/x/exp/slices"

// Committer is the interface supporting serializing tries into persistent storage.
type Committer interface {
StorePage(page uint64, content []byte) error
Expand All @@ -40,9 +42,7 @@ func (mc *InMemoryCommitter) StorePage(page uint64, content []byte) error {
if content == nil {
delete(mc.memStore, page)
} else {
storedContent := make([]byte, len(content))
copy(storedContent, content)
mc.memStore[page] = storedContent
mc.memStore[page] = slices.Clone(content)
}
return nil
}
Expand Down
5 changes: 2 additions & 3 deletions crypto/merkletrie/committer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"testing"

"github.com/stretchr/testify/require"
"golang.org/x/exp/slices"

"github.com/algorand/go-algorand/crypto"
"github.com/algorand/go-algorand/test/partitiontest"
Expand All @@ -33,9 +34,7 @@ func (mc *InMemoryCommitter) Duplicate(flat bool) (out *InMemoryCommitter) {
if flat {
out.memStore[k] = v
} else {
bytes := make([]byte, len(v))
copy(bytes[:], v[:])
out.memStore[k] = bytes
out.memStore[k] = slices.Clone(v)
jannotti marked this conversation as resolved.
Show resolved Hide resolved
}
}
return
Expand Down
4 changes: 2 additions & 2 deletions crypto/merkletrie/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"unsafe"

"github.com/algorand/go-algorand/crypto"
"golang.org/x/exp/slices"
)

type childEntry struct {
Expand Down Expand Up @@ -339,8 +340,7 @@ func deserializeNode(buf []byte) (n *node, s int) {
if hashLength2 <= 0 {
return nil, hashLength2
}
n.hash = make([]byte, hashLength)
copy(n.hash, buf[hashLength2:hashLength2+int(hashLength)])
n.hash = slices.Clone(buf[hashLength2 : hashLength2+int(hashLength)])
s = hashLength2 + int(hashLength)
isLeaf := (buf[s] == 0)
s++
Expand Down
4 changes: 2 additions & 2 deletions daemon/algod/api/server/v2/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/algorand/go-algorand/crypto"
"github.com/algorand/go-algorand/daemon/algod/api/server/v2/generated/model"
"github.com/algorand/go-algorand/data/basics"
"golang.org/x/exp/slices"
)

// AssetHolding converts between basics.AssetHolding and model.AssetHolding
Expand Down Expand Up @@ -477,8 +478,7 @@ func AssetParamsToAsset(creator string, idx basics.AssetIndex, params *basics.As
Reserve: addrOrNil(params.Reserve),
}
if params.MetadataHash != ([32]byte{}) {
metadataHash := make([]byte, len(params.MetadataHash))
copy(metadataHash, params.MetadataHash[:])
metadataHash := slices.Clone(params.MetadataHash[:])
assetParams.MetadataHash = &metadataHash
}

Expand Down
4 changes: 2 additions & 2 deletions daemon/algod/api/server/v2/test/handlers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (

"github.com/algorand/go-algorand/ledger/eval"
"github.com/algorand/go-algorand/ledger/ledgercore"
"golang.org/x/exp/slices"

"github.com/labstack/echo/v4"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -1028,8 +1029,7 @@ int 1`,

var expectedFailedAt *[]uint64
if len(scenario.FailedAt) != 0 {
clone := make([]uint64, len(scenario.FailedAt))
copy(clone, scenario.FailedAt)
clone := slices.Clone(scenario.FailedAt)
clone[0]++
expectedFailedAt = &clone
}
Expand Down
4 changes: 2 additions & 2 deletions daemon/algod/api/server/v2/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

"github.com/algorand/go-codec/codec"
"github.com/labstack/echo/v4"
"golang.org/x/exp/slices"

"github.com/algorand/go-algorand/daemon/algod/api/server/v2/generated/model"
"github.com/algorand/go-algorand/data/basics"
Expand Down Expand Up @@ -431,8 +432,7 @@
}

if len(txnGroupResult.FailedAt) > 0 {
failedAt := make([]uint64, len(txnGroupResult.FailedAt))
copy(failedAt, txnGroupResult.FailedAt)
failedAt := slices.Clone[[]uint64, uint64](txnGroupResult.FailedAt)

Check warning on line 435 in daemon/algod/api/server/v2/utils.go

View check run for this annotation

Codecov / codecov/patch

daemon/algod/api/server/v2/utils.go#L435

Added line #L435 was not covered by tests
encoded.FailedAt = &failedAt
}

Expand Down
29 changes: 3 additions & 26 deletions data/basics/teal.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
"fmt"

"github.com/algorand/go-algorand/config"
"golang.org/x/exp/maps"
)

// DeltaAction is an enum of actions that may be performed when applying a
Expand Down Expand Up @@ -77,24 +78,7 @@
// equality because an empty map will encode/decode as nil. So if our generated
// map is empty but not nil, we want to equal a decoded nil off the wire.
func (sd StateDelta) Equal(o StateDelta) bool {
// Lengths should be the same
if len(sd) != len(o) {
return false
}
// All keys and deltas should be the same
for k, v := range sd {
// Other StateDelta must contain key
ov, ok := o[k]
if !ok {
return false
}

// Other StateDelta must have same value for key
if ov != v {
return false
}
}
return true
return maps.Equal(sd, o)
Copy link
Contributor Author

@jannotti jannotti Jun 20, 2023

Choose a reason for hiding this comment

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

Comment goes into a lot of detail about the nil checking choice, but that is exactly the same choice that maps.Equal made. Unit tests already exist to confirm.

}

// Valid checks whether the keys and values in a StateDelta conform to the
Expand Down Expand Up @@ -234,14 +218,7 @@
// Clone returns a copy of a TealKeyValue that may be modified without
// affecting the original
func (tk TealKeyValue) Clone() TealKeyValue {
if tk == nil {
return nil
}
res := make(TealKeyValue, len(tk))
for k, v := range tk {
res[k] = v
}
return res
return maps.Clone(tk)

Check warning on line 221 in data/basics/teal.go

View check run for this annotation

Codecov / codecov/patch

data/basics/teal.go#L221

Added line #L221 was not covered by tests
}

// ToStateSchema calculates the number of each value type in a TealKeyValue and
Expand Down