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: Small cleanups for clarity #5526

Merged
merged 2 commits into from Jul 6, 2023

Conversation

jannotti
Copy link
Contributor

@jannotti jannotti commented Jul 5, 2023

I'm doing a larger bit of work to enable logicsig cost pooling and I found myself making these changes along the way. It's easier to review by extracting them into their own PR. This does four things.

  1. Introduces and uses:
func (msig MultisigSig) Signatures() int

for the several places we need to examine a MultiSig to count the number of sub signatures present.

  1. Uses the pre-existing func (s *Signature) Blank() bool in many places.

  2. Renames EvalContext.runModeFlags to simply EvalContext.runMode because it takes on a single value, it can't be, for example: ModeSig | ModeApp, it must be one or the other.

  3. Improves tests by making it so that sample transactions created for versions below rekeyingEnabledVerion don't have their RekeyAddress set.

@jannotti jannotti changed the title Small cleanups for clarity Chore: Small cleanups for clarity Jul 5, 2023
@jannotti jannotti self-assigned this Jul 5, 2023
I'm doing a larger bit of work to enable logicsig cost pooling and I
found myself making these changes along the way. It's easier to review
by extracting them into their own PR.  This does three things.

Introduces and uses:
```
func (msig MultisigSig) Signatures() int
```
for the sevral places we need to examine a MultiSig to count the
number of sub signatures present.

Uses the pre-existing `func (s *Signature) Blank() bool` in many
places.

Renames `EvalContext.runModeFlags` to simply `EvalContext.runMode`
because it takes on a single value, it can't be, for example: `ModeSig |
ModeApp`, it must be one or the other.
@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #5526 (ebb4f0c) into master (22e0e09) will decrease coverage by 0.02%.
The diff coverage is 82.97%.

@@            Coverage Diff             @@
##           master    #5526      +/-   ##
==========================================
- Coverage   55.81%   55.79%   -0.02%     
==========================================
  Files         446      446              
  Lines       63228    63213      -15     
==========================================
- Hits        35290    35271      -19     
  Misses      25572    25572              
- Partials     2366     2370       +4     
Impacted Files Coverage Δ
data/transactions/verify/txn.go 55.72% <75.00%> (-1.00%) ⬇️
crypto/multisig.go 51.07% <77.77%> (+1.07%) ⬆️
data/transactions/logic/eval.go 92.31% <88.23%> (ø)
data/transactions/logic/debugger.go 69.86% <100.00%> (ø)
data/transactions/verify/txnBatch.go 98.24% <100.00%> (-0.08%) ⬇️

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ahangsu
ahangsu previously approved these changes Jul 5, 2023
Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

looks good

jasonpaulos
jasonpaulos previously approved these changes Jul 5, 2023
Copy link
Member

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Good improvements

crypto/multisig.go Outdated Show resolved Hide resolved
@jannotti jannotti dismissed stale reviews from jasonpaulos and ahangsu via ebb4f0c July 5, 2023 17:49
@jannotti jannotti marked this pull request as ready for review July 6, 2023 17:43
@jannotti jannotti merged commit 3e80027 into algorand:master Jul 6, 2023
17 checks passed
@jannotti jannotti deleted the logicsig-pooling-prep branch July 6, 2023 17:43
ASISBusiness

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants