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

tools: prepare block-generator for configuring apps #5443

Merged
merged 9 commits into from Jun 9, 2023

Conversation

tzaffi
Copy link
Contributor

@tzaffi tzaffi commented Jun 2, 2023

Summary

Adding functionality to handle app transaction configuration, but NOT generating such transactions. A followup PR uses this functionality in generating app transactions.

Breaking out block generator configuration related code into

  • tools/block-generator/config.go
  • tools/block-generator/config_test.go

Blocked by

Test Plan

This is only tested in CI. However, a followup PR that includes all this code is passing local end-to-end testing.

// TX Distribution / ID's
paymentTx TxTypeID = "pay"
assetTx TxTypeID = "asset"
applicationTx TxTypeID = "appl"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was un-commented

assetXfer TxTypeID = "asset_xfer"
assetClose TxTypeID = "asset_close"

// App kind TX Distribution / ID's don't exist because these are flattened
Copy link
Contributor Author

Choose a reason for hiding this comment

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

all the rest of the constants are new

// TX Distribution
PaymentTransactionFraction float32 `yaml:"tx_pay_fraction"`
AssetTransactionFraction float32 `yaml:"tx_asset_fraction"`
AppTransactionFraction float32 `yaml:"tx_app_fraction"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is new

Comment on lines +177 to +197
// App kind TX Distribution
AppSwapFraction float32 `yaml:"app_swap_fraction"`
AppBoxesFraction float32 `yaml:"app_boxes_fraction"`

// App Swap TX Distribution
AppSwapCreateFraction float32 `yaml:"app_swap_create_fraction"`
AppSwapUpdateFraction float32 `yaml:"app_swap_update_fraction"`
AppSwapDeleteFraction float32 `yaml:"app_swap_delete_fraction"`
AppSwapOptinFraction float32 `yaml:"app_swap_optin_fraction"`
AppSwapCallFraction float32 `yaml:"app_swap_call_fraction"`
AppSwapCloseFraction float32 `yaml:"app_swap_close_fraction"`
AppSwapClearFraction float32 `yaml:"app_swap_clear_fraction"`

// App Boxes TX Distribution
AppBoxesCreateFraction float32 `yaml:"app_boxes_create_fraction"`
AppBoxesUpdateFraction float32 `yaml:"app_boxes_update_fraction"`
AppBoxesDeleteFraction float32 `yaml:"app_boxes_delete_fraction"`
AppBoxesOptinFraction float32 `yaml:"app_boxes_optin_fraction"`
AppBoxesCallFraction float32 `yaml:"app_boxes_call_fraction"`
AppBoxesCloseFraction float32 `yaml:"app_boxes_close_fraction"`
AppBoxesClearFraction float32 `yaml:"app_boxes_clear_fraction"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the rest of these app-related fields are new

Comment on lines +205 to +218
func initializeConfigFile(configFile string) (config GenerationConfig, err error) {
var data []byte
data, err = os.ReadFile(configFile)
if err != nil {
return
}
err = yaml.Unmarshal(data, &config)
if err != nil {
return
}

err = config.validateWithDefaults(true)
return
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved from generate.go and using validateWithDefaults() to fail fast.

// sumIsCloseToOneWithDefault returns no error if the sum of the params is close to 1.
// It returns an error if any of the params are negative.
// Finally, in the case that all the params are zero, it sets the first param to 1 and returns no error.
func sumIsCloseToOneWithDefault(defaults bool, params ...*float32) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sumIsCloseToOneWithDefault and validateSumCloseToOne replace sumIsCloseToOne and add the ability to default missing sections.

Copy link
Contributor

Choose a reason for hiding this comment

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

Defaults is so you don't have to set anything for unused types? Maybe it could be 1 or 0?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do they need to be pointers? it would be nice to avoid asPtrSlice everywhere.

Copy link
Contributor Author

@tzaffi tzaffi Jun 8, 2023

Choose a reason for hiding this comment

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

Defaults is so you don't have to set anything for unused types? Maybe it could be 1 or 0?

When an entire section is 0-valued, and defaults == true the first pointer is set to 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do they need to be pointers? it would be nice to avoid asPtrSlice everywhere.

Because these when defaults == true they modify the given values so I need to pass by pointer.

@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Merging #5443 (5099dfa) into master (d5593a4) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5443   +/-   ##
=======================================
  Coverage   55.61%   55.62%           
=======================================
  Files         447      447           
  Lines       63395    63395           
=======================================
+ Hits        35259    35264    +5     
+ Misses      25757    25754    -3     
+ Partials     2379     2377    -2     

see 6 files with indirect coverage changes

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

@tzaffi tzaffi marked this pull request as ready for review June 2, 2023 18:16
@tzaffi tzaffi requested a review from a team June 2, 2023 18:17
tools/block-generator/generator/config.go Outdated Show resolved Hide resolved
app_swap_optin_fraction: 0.1
app_swap_call_fraction: 0.98
app_swap_close_fraction: 0.005
app_swap_clear_fraction: 0.003
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please elaborate what you have mind for an app swap scenario? are these apps using the swap opcode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is out of context. #5450 sheds a bit of light. In particular, I'm creating two apps with teal code:

  1. for populating boxes
  2. for swapping assets using inner transactions

That PR doesn't actually make boxes / swap assets yet, but it does create the apps and set some local state.

tools/block-generator/README.md Show resolved Hide resolved
Copy link
Contributor

@shiqizng shiqizng 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.

@shiqizng shiqizng requested a review from a team June 6, 2023 18:41
@tzaffi tzaffi requested a review from shiqizng June 7, 2023 18:27
winder
winder previously approved these changes Jun 8, 2023
@tzaffi tzaffi requested a review from winder June 9, 2023 14:14
@winder winder merged commit 5ff0c22 into algorand:master Jun 9, 2023
25 checks passed
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

3 participants