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

Sketch a simple facade: the entrypoint #69

Merged
merged 17 commits into from
Jul 25, 2024
Merged

Sketch a simple facade: the entrypoint #69

merged 17 commits into from
Jul 25, 2024

Conversation

andreibancioiu
Copy link
Contributor

@andreibancioiu andreibancioiu commented May 30, 2024

Define some higher-level components (e.g. entrypoint, Account, controllers) over the low-level ones.

  • Useful for getting developers on-board.
  • Useful for tutorials
  • Useful for scripting and back-end development
  • Might also be useful in dApps (e.g. for contract interactions)

@andreibancioiu andreibancioiu marked this pull request as draft May 30, 2024 13:37
@andreibancioiu andreibancioiu changed the base branch from main to queries-30 May 30, 2024 13:37
@andreibancioiu andreibancioiu self-assigned this May 30, 2024
Base automatically changed from queries-30 to main June 4, 2024 13:00
@andreibancioiu andreibancioiu changed the title Sketch a simple facade: the main facade Sketch a simple facade: the entrypoint Jun 11, 2024
Comment on lines +21 to +35
// Named constructor
// Loads a secret key from a PEM file. PEM files may contain multiple accounts - thus, an (optional) "index" is used to select the desired secret key.
// Returns an Account object, initialized with the secret key.
static new_from_pem(path: Path, index: Optional[int], hrp: Optional[str]): Account;

// Named constructor
// Loads a secret key from an encrypted keystore file. Handles both keystores that hold a mnemonic and ones that hold a secret key (legacy).
// For keystores that hold an encrypted mnemonic, the optional "address_index" parameter is used to derive the desired secret key.
// Returns an Account object, initialized with the secret key.
static new_from_keystore(path: Path, password: str, address_index: Optional[int], hrp: Optional[str]): Account;

// Named constructor
// Loads (derives) a secret key from a mnemonic. The optional "address_index" parameter is used to guide the derivation.
// Returns an Account object, initialized with the secret key.
static new_from_mnemonic(mnemonic: str, address_index: Optional[int], hrp: Optional[str]): Account;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have new_from_wallet as a single method, or separate, explicit functions (as seen here)?

class NetworkEntrypoint:
// The constructor is not captured by the specs; it's up to the implementing library to define it.
// For example, it can be parametrized with: a network provider URL and kind (proxy, API)
constructor(url: str, kind: Optional[str]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the suggested constructor is all right?

If kind is missing, we'd like to infer the kind of network provider based on its URL.

Comment on lines +75 to +83
// Broadcast transaction (option 1):
transaction_hash = entrypoint.send_transaction(transaction);
// Broadcast transaction (option 2):
transaction_hash = entrypoint.get_network_provider().send_transaction(transaction);

// Await transaction completion (option 1, await and specialized outcome parsing):
parsed_outcome = controller.await_completed_transfer(transaction_hash);
// Await transaction completion (option 2, simple await):
transaction_on_network = entrypoint.await_completed_transaction(transaction_hash);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if creation (and signing) of transactions is done by controllers, broadcasting isn't (see examples).

All good? Alternative: have send_transaction() in each controller, instead of having it in the entrypoint?

Choose a reason for hiding this comment

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

I think it's ok as is - maybe if we add them into controller would open discussions about consistency, eg: "why is send_transaction inside the controller but other network related suff like recall_account_nonce is in the entrypoint?"

Choose a reason for hiding this comment

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

  1. Maybe I'm too nitpicky and I keep coming back to old talks that maybe we already concluded, but like in this example, we get the get_transfers_controller only to use it once to create a transaction, then never use it again (cause everywhere else we're using the entrypoint).
    I will most probably just do entrypoint.get_transfers_controller().create_transaction_for_transfer() to shorten it for single use only and it wouldn't be the end of the world, but the idea of creating that intermediary object to get to the transaction is nagging me ever so slightly, and this leads me to the second point:

  2. Is that a controller? Since it still looks more like a factory, just different in the way that I take it directly from the entrypoint rather than importing it myself.
    Much easier to get and use than the base sdk intricacies, nevertheless.

  3. One thing as food for thought that I keep wondering for a while and asked before but maybe lost its point is whether we need to have separate entities for Address and Account. I would rather use Account everywhere with the particularity that some can have signing abilities (if private key is provided to them), others don't (if it only has a public key available like: Account.new_from_bech32("blabla")).
    The reason I'm picking at this is because at chain level they're the same entity, but they're not interchangeable in code and they introduce a debateable unnecessary friction in the fact that I have to import them separately and use them differently based on where they go (if I'm a sender, I need to use the account; if i'm the receiver, I have to use the address). Not much effort involved into this, I know, but still an effort that has to be explained and understood.

  4. Should we stick to entrypoint name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Fair point. In this last version we've thought of not having functions that create transactions directly on the entrypoint, but in the child controllers, instead. Debatable, though 👍

  2. Indeed, it's a controller. The more lower-level factory does not returned signed transactions (the controller signs the transactions). The controller is also able to do a "specialized" await for a transaction's completion - await & properly parse its outcome (the lower level fashion is, now, to await for transaction completion using a transaction awaiter, then use a transaction outcome parser to recover & interpret returned data - e.g. logs).

  3. I'd like to keep Address a different concept, as it is now. Especially because I'm thinking of Brainstorm design of Address #36.

  4. Somehow better than facade.

Choose a reason for hiding this comment

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

  1. Understood the point and wasn't aware of those things without seeing the background. For example, entrypoint.await_completed_transaction leave the impression that the await is done here, while the signing is invisible, so the only thing the dev sees is the factory/builder element :)
    Maybe we complicate the names too much with the patterns, don't know.

Copy link
Contributor Author

@andreibancioiu andreibancioiu Jun 19, 2024

Choose a reason for hiding this comment

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

Noted 👍

Additional note: entrypoint.await_completed_transaction does a generic (non-specific) await for transaction completion. The controllers, instead, are more specialized (more informed) in this regard e.g. controller.await_completed_execute() parses the contract outcome (return data) and returns a specific data structure, not a mere TransactionOnNetwork. Another example, for the tokens controller: controller.await_completed_issue_fungible, which not only waits for transaction completion, but also returns data such as the identifier of the newly registered token etc.

Copy link

@ovidiuolteanu ovidiuolteanu Jun 19, 2024

Choose a reason for hiding this comment

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

oh God. I completely missed this. I think this is reopening up @ccorcoveanu's comment above for debate on consistency. Let's talk in a meet.

@andreibancioiu andreibancioiu marked this pull request as ready for review June 11, 2024 09:59
get_account_management__controller(): AccountManagementController;
```

## Examples
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you feel about the examples?

Choose a reason for hiding this comment

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

All good from my pov 👍

Comment on lines +75 to +83
// Broadcast transaction (option 1):
transaction_hash = entrypoint.send_transaction(transaction);
// Broadcast transaction (option 2):
transaction_hash = entrypoint.get_network_provider().send_transaction(transaction);

// Await transaction completion (option 1, await and specialized outcome parsing):
parsed_outcome = controller.await_completed_transfer(transaction_hash);
// Await transaction completion (option 2, simple await):
transaction_on_network = entrypoint.await_completed_transaction(transaction_hash);

Choose a reason for hiding this comment

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

  1. Maybe I'm too nitpicky and I keep coming back to old talks that maybe we already concluded, but like in this example, we get the get_transfers_controller only to use it once to create a transaction, then never use it again (cause everywhere else we're using the entrypoint).
    I will most probably just do entrypoint.get_transfers_controller().create_transaction_for_transfer() to shorten it for single use only and it wouldn't be the end of the world, but the idea of creating that intermediary object to get to the transaction is nagging me ever so slightly, and this leads me to the second point:

  2. Is that a controller? Since it still looks more like a factory, just different in the way that I take it directly from the entrypoint rather than importing it myself.
    Much easier to get and use than the base sdk intricacies, nevertheless.

  3. One thing as food for thought that I keep wondering for a while and asked before but maybe lost its point is whether we need to have separate entities for Address and Account. I would rather use Account everywhere with the particularity that some can have signing abilities (if private key is provided to them), others don't (if it only has a public key available like: Account.new_from_bech32("blabla")).
    The reason I'm picking at this is because at chain level they're the same entity, but they're not interchangeable in code and they introduce a debateable unnecessary friction in the fact that I have to import them separately and use them differently based on where they go (if I'm a sender, I need to use the account; if i'm the receiver, I have to use the address). Not much effort involved into this, I know, but still an effort that has to be explained and understood.

  4. Should we stick to entrypoint name?

```
entrypoint = MainnetEntrypoint();
abi = Abi.load("adder.abi.json");
controller = entrypoint.create_smart_contract_controller(abi);

Choose a reason for hiding this comment

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

I believe this is what was bugging me:
create_smart_contract_controller
get_transfers_controller

They have the same purpose in the end but it's not directly intuitive which should be which and we got to this situation in which we actually ended up with both: one with get, another with create.
Even though I am aware that this may happened by mistake because of it being a controller/factory/whatever in the lower layer, this kinda proves my point that I may look for any of those because of this pattern.
And makes me wish that I just could've just done entrypoint.smart_contract_controller.get_me_a_call()

Copy link
Contributor Author

@andreibancioiu andreibancioiu Jun 19, 2024

Choose a reason for hiding this comment

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

Other controllers - e.g. token management controller, delegation controller - do not have the dependency on "ABI" when created. Their lifetime is managed by the entrypoint itself. The entrypoint creates them once (when initialized), then provides their references when requested by client code.

However, the controller for a smart contract is explicitly instantiated - thus the "create" verb. The caller (client code) provides the ABI.

Copy link

@ovidiuolteanu ovidiuolteanu Jun 19, 2024

Choose a reason for hiding this comment

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

I see now, but only because you explained it.
In honesty, this would annoy me quite badly because of never knowing what do I have to look for intuitively.

The mental effort for this is too much in my opinion if we're forcing the devs to differentiate a generic thing (creation of a tx) based on the way we implemented the controller. Sorry, but I don't see this as simple enough design.

I think it's one of the differences we had in the lower levels as well, e.g. receiver member vs destination. It's an unnecessary complication in my opinion.

Copy link
Contributor Author

@andreibancioiu andreibancioiu Jun 19, 2024

Choose a reason for hiding this comment

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

Noted 👍

We can have this with get instead of create, as well, and require the ABI when calling create_transaction_for_execute, query etc. - as it was the idea at first.

Choose a reason for hiding this comment

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

Yeah... not very nice, I guess... I would rather instantiate the other controllers only when called, similarly to the contract controller, then call all of them create as well... I don't know. I give up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted to use create_* for controllers.

popenta
popenta previously approved these changes Jul 16, 2024
danielailie
danielailie previously approved these changes Jul 16, 2024
@andreibancioiu andreibancioiu merged commit f4c7c95 into main Jul 25, 2024
@andreibancioiu andreibancioiu deleted the facades-01 branch July 25, 2024 08:13
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.

5 participants