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

Added library config #160

Merged
merged 3 commits into from
Nov 29, 2024
Merged

Added library config #160

merged 3 commits into from
Nov 29, 2024

Conversation

popenta
Copy link
Collaborator

@popenta popenta commented Nov 29, 2024

Added a LibraryConfig that stores the default hrp value.

@popenta popenta self-assigned this Nov 29, 2024
Copy link

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  multiversx_sdk
  __init__.py
  multiversx_sdk/converters
  transactions_converter.py
  multiversx_sdk/core
  __init__.py
  address.py
  config.py
  constants.py
  multiversx_sdk/core/transaction_builders
  default_configuration.py
  relayed_v1_builder.py
  multiversx_sdk/core/transactions_factories
  delegation_transactions_factory.py
  transactions_factory_config.py
  multiversx_sdk/core/transactions_outcome_parsers
  delegation_transactions_outcome_parser.py
  smart_contract_transactions_outcome_parser.py
  token_management_transactions_outcome_parser.py
  multiversx_sdk/network_providers
  api_network_provider.py
  proxy_network_provider.py 164
  transactions.py
  multiversx_sdk/wallet
  user_keys.py
  user_wallet.py
Project Total  

This report was generated by python-coverage-comment-action

andreibancioiu
andreibancioiu previously approved these changes Nov 29, 2024
"source": [
"### Changing the default hrp\n",
"\n",
"We have a configuration class, called `LibraryConfig`, that only stores the **default hrp** of the addresses. The default value is `erd`. The hrp can be changed when instantiating an address, or it can be changed in the `LibraryConfig` class, and all the addresses created will have the newly set hrp. "
Copy link
Contributor

Choose a reason for hiding this comment

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

that only stores ... - "for now" / "for the moment".

Maybe, in the future, we'll move the most of the constants (that we use for default values in various parts). That way, the constants will be "promoted" to static variables.

@@ -36,7 +37,7 @@ def __init__(self, pubkey: bytes, hrp: str) -> None:
raise ErrBadPubkeyLength(len(pubkey), PUBKEY_LENGTH)

self.pubkey = bytes(pubkey)
self.hrp = hrp
self.hrp = hrp if hrp else LibraryConfig.default_address_hrp
Copy link
Contributor

Choose a reason for hiding this comment

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

All right, good change.

Generally speaking, this configuration should only be altered on exotic use cases
it can be seen as a collection of constants (or , to be more precise, rarely changed variables) that are used throughout the library.

Never alter the configuration within a library!
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. Only in an end application.

Altering within a library (several times) might lead to ambiguity and conflicts.

it can be seen as a collection of constants (or , to be more precise, rarely changed variables) that are used throughout the library.

Never alter the configuration within a library!
Only alter the configuration(if needed) within an(end) application that uses this library.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo / spacing.

DELEGATION_MANAGER_SC_ADDRESS = "erd1qqqqqqqqqqqqqqqpqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqylllslmq6y6"

# left it for compatibility reasons, will be deleted in the future
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

config: Optional[NetworkProviderConfig] = None
) -> None:
self.url = url
self.auth = auth
self.address_hrp = address_hrp
self.address_hrp = address_hrp or DEFAULT_ADDRESS_HRP
self.address_hrp = address_hrp or LibraryConfig.default_address_hrp
Copy link
Contributor

Choose a reason for hiding this comment

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

👍



# this is duplicated code, added here to get rid of a circular dependency
# will be removed in V1
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@popenta popenta merged commit 057c29f into main Nov 29, 2024
7 checks passed
@popenta popenta deleted the library-config branch November 29, 2024 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants