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

Add default address encodings for Bitcoin #50

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sidhujag
Copy link
Contributor

@sidhujag sidhujag commented Dec 3, 2020

This should be here so its easily overridable by other UTXO chains.

@@ -85,35 +85,53 @@ const (
P2PKHScriptPubkeySize = 25 // P2PKH size
)

// CreateMainNetParams is a function to override default mainnet settings with address prefixes
func CreateMainNetParams() (*chaincfg.Params) {
Copy link
Contributor

@patrick-ogrady patrick-ogrady Dec 3, 2020

Choose a reason for hiding this comment

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

@sidhujag would it make more sense to provide a JSON file at initialization that determines the values of chaincfg.Params? If not provided, it would just default to what we already had here.

In the general case, I think this would mean that a fork could integrate without maintaining their own repo? Unless you've seen other things that must be changed?

Copy link
Contributor Author

@sidhujag sidhujag Dec 3, 2020

Choose a reason for hiding this comment

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

Yup that makes sense. Outside of these network changes and the overriding of the default table/log sizes for badger we should be able to just use rosetta-bitcoin. I actually prefer this option. If we can move it out to json then we can configure network settings, the genesis details, the address prefixes and it should just work.

See syscoin#1 for related issue, the CLI also needs configurable setting for the badger log/table defaults. Other than that it is common with the base.

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

Successfully merging this pull request may close these issues.

2 participants