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 entrypoints #514

Closed
wants to merge 25 commits into from
Closed

Add entrypoints #514

wants to merge 25 commits into from

Conversation

danielailie
Copy link
Contributor

No description provided.

@danielailie danielailie changed the title Add controllers si facades Add entrypoints Oct 28, 2024
@danielailie danielailie self-assigned this Oct 28, 2024
@andreibancioiu andreibancioiu self-requested a review October 28, 2024 15:57
@popenta popenta self-requested a review October 29, 2024 08:19
src/account.ts Outdated
@@ -15,18 +18,30 @@ export class Account {
*/
nonce: INonce = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know, we are using bigint for nonce. Would it be a breaking change if we'd do that here?

src/account.ts Outdated
return this.signer.sign(data);
}

static fromPem(path: string, hrp: string = LibraryConfig.DefaultAddressHrp): Account {
Copy link
Contributor

Choose a reason for hiding this comment

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

in the specs, the static methods are prefixed with new...

Copy link
Contributor

Choose a reason for hiding this comment

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

new_from_mnemonic is also missing.

@@ -1,11 +1,11 @@
import BigNumber from "bignumber.js";
import { ITransactionOnNetwork } from "./interfaceOfNetwork";
import { TransactionOnNetwork } from "./networkProviders";
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure we should use objects from networkProviders in the core package. Perhaps we should keep the interface or move the TransactionOnNetwork in core. Would that be breaking?

Comment on lines +19 to +21
nonce: number;
round: number;
epoch: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use bigint instead of number.

function?: string;
data: Buffer;
signature: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

and maybe have the signature as UInt8Array

@@ -376,6 +376,30 @@ export class DelegationTransactionsFactory {
}).build();
}

createTransactionForWithdrawing(_options: { sender: IAddress; delegationContract: IAddress }): Transaction {
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps add some empty lines, but it's not a big deal.

@@ -0,0 +1,64 @@
import { IAddress } from "../interface";
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the methods pubkey() and bech32() are deprecated in the Address class, I think we can create locally a new interface that has toBech32() and getPublicKey(), given the fact that everybody will use that class.

return transaction;
}

async createTransactionForExecute(sender: IAccount, options: TransactionInput): Promise<Transaction> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could've implemented parseExecute and awaitExecuteCompleted.


parseIssueFungible(transactionOnNetwork: TransactionOnNetwork): IESDTIssueOutcome[] {
return this.parser.parseIssueFungible(transactionOnNetwork);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

add empty line

Copy link
Contributor

Choose a reason for hiding this comment

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

Why didn't you implement all the methods that are present in TokenManagementTransactionsFactory?

Copy link
Contributor

@andreibancioiu andreibancioiu left a comment

Choose a reason for hiding this comment

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

🚀 🌔

import { ProxyNetworkProvider, TransactionOnNetwork } from "../networkProviders";
import { INetworkProvider } from "../networkProviders/interface";

// This will be deleted in future version
Copy link
Contributor

Choose a reason for hiding this comment

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

We can drop it now, in this PR, after applying the following change:

multiversx/mx-sdk-js-network-providers#74

chainId: string;
}

export class TestnetEntrypointConfig implements EntrypointConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding localnet, as well?

class LocalnetEntrypointConfig:
    network_provider_url = "http://localhost:7950"
    network_provider_kind = "proxy"
    chain_id = "localnet"

src/account.ts Outdated
@@ -15,18 +18,30 @@ export class Account {
*/
nonce: INonce = 0;

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Very good that we now have these deprecation markers.

src/account.ts Outdated
/**
* The signer of the account.
*/
signer?: UserSigner;
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, let's switch to private access modifier (so that we hide the current workaround of having a "frankenstein" Account).

src/account.ts Outdated
return new Account(userSigner.getAddress(hrp), userSigner);
}

static fromWallet(
Copy link
Contributor

Choose a reason for hiding this comment

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

Below (and in PY & specs), we use the term keystore. Perhaps this can also be named fromKeystoreObject (parameters can also be renamed)?

private networkProvider: ApiNetworkProvider | ProxyNetworkProvider;
private chainId: string;

constructor(networkProviderUrl: string, networkProviderKind: string, chainId: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Options pattern?

private networkProvider: ApiNetworkProvider | ProxyNetworkProvider;
private chainId: string;

constructor(networkProviderUrl: string, networkProviderKind: string, chainId: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In TypeScript, we can have networkProviderKind as an enum or have the type annotation as "proxy" | "api" (can also stay as it is).

return new RelayedController(this.chainId);
}

createSmartContractController(abi?: AbiRegistry): SmartContractController {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can have an alias Abi, for example:

class Abi extends AbiRegistry;

import { TransactionWatcher } from "../transactionWatcher";
import { IAccount } from "./interfaces";

export class TokenManagementController {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems incomplete. But we can add remaining functionality in future PRs.

import { DevnetEntrypoint } from "./entrypoints";

describe("TestEntrypoint", () => {
const entrypoint = new DevnetEntrypoint();
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀

@danielailie danielailie changed the base branch from feat/unify to main October 30, 2024 12:00
@danielailie danielailie changed the base branch from main to feat/next November 13, 2024 11:19
@danielailie
Copy link
Contributor Author

this was done in smaller prs

@danielailie danielailie deleted the TOOL-276-implement-facade branch December 19, 2024 13: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