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

feat!: fetch and cache chain info on Provider when initialized #1181

Merged
merged 74 commits into from
Sep 19, 2023

Conversation

Dhaiwat10
Copy link
Member

@Dhaiwat10 Dhaiwat10 commented Aug 9, 2023

Summary

This PR caches the chainInfo for a given Fuel node on the Provider instance whenever a Provider is initialized.

Note: I know this PR has a lot of changes (~ 80 files), but 90% of these changes are changes made to our test files so that they are adapted to the breaking changes. I recommend focusing on reviewing these files: provider.ts, script-invocation-scope.ts, base-invocation-scope.ts.

Motivation

Please see #1176 for a detailed breakdown of the problem at hand, but tldr: we are currently using a lot of hardcoded values for some constants that we use when creating transactions. These hardcoded values served us well until now, but we now need to fetch them from the chain dynamically. This has been requested as a top priority by the frontend team.

Breaking Changes

Because of the nature of the changes introduced in this PR, and the fact that Provider is used so widely in our SDK, there are multiple breaking changes that we need to be aware of and need to communicate to our users:

1. Initializing the Provider

// Used to be:
const provider = new Provider(url)

// New API:
const provider = await Provider.create(url)

2. Wallet methods

All of these methods now require a Provider to be passed in:

const provider = await Provider.create(url);

// Some of these methods used to accept a URL instead of a `Provider` object.
// Note that the `provider` param *has* to be a `Provider` object now.
WalletUnlocked.fromSeed(seed, provider, path);
WalletUnlocked.fromMnemonic(mnemonic, provider, path, passphrase);
WalletUnlocked.fromExtendedKey(extendedKey, provider);
await WalletUnlocked.fromEncryptedJson(jsonWallet, password, provider);

Wallet.fromAddress(address, provider);
Wallet.fromPrivateKey(pk, provider);

Wallet.generate({ provider });

3. Account class

new Account(address, provider);

4. PrivateKeyVault

// these are the options that are accepted by the `PrivateKeyVault` constructor. `provider` is a required input now.
interface PkVaultOptions {
  secret?: string;
  accounts?: Array<string>;
  provider: Provider;
}

5. MnemonicVault

Same as PrivateKeyVault. The constructor requires a provider to be passed in.

6. WalletManager

The VaultConfig now requires a provider to be passed in.

export type VaultConfig = {
  type: string;
  title?: string;
  secret?: string;
  provider: Provider;
};

7. Predicate

// `provider` is no longer optional. there is a change in order of the parameters here, + we don't require a `chainId` to be passed anymore. 
new Predicate(bytes, provider, jsonAbi);

Other notes

To demonstrate the fact that we can now use fresh on-chain values for some of the previosuly hard-coded constants, I have refactored the usage of the VM_TX_MEMORY constant. This constant is used to calculate the contract call script's data offset.

Closes #1176
Closes #1260

@Dhaiwat10 Dhaiwat10 added the feat Issue is a feature label Aug 9, 2023
@Dhaiwat10 Dhaiwat10 self-assigned this Aug 9, 2023
Copy link
Contributor

@danielbate danielbate left a comment

Choose a reason for hiding this comment

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

Looking good, I am in favour of bringing us closer to the Rust SDK and improving the UX. However couple comments on the approach:

  • If connect() is now paramount to populate consensusParameters, we should make the constructor private and therefore Provider becomes instantiatable only via connect(). Therefore we won't need to encourage one way or the other and it is enforced. We could introduce a ProviderFactory to handle the provision of a packaged provider but this is potentially an unnecessary level of abstraction if we are in preference of dev ex and bringing us closer to the Rust SDK.
  • Or we maintain the API, and call connect() from within the constructor (not my preference but lots of breaking changes / tutorials to think of).
  • Another thought is that now consensusParameters and the provider itself are now tightly bound. Should we be making url read only, and restricting the ability to update a url as that itself should refetch the params. Or we make the url update in turn refetch. But I feel as if we are restricting those, we should just be reinstantiating a Provider class?

packages/program/src/functions/base-invocation-scope.ts Outdated Show resolved Hide resolved
@Dhaiwat10
Copy link
Member Author

A bunch of spaghetti code still, don't review yet pls :P

@camsjams camsjams changed the title feat: fetch and cache consensus params on Provider when initialized feat!: fetch and cache consensus params on Provider when initialized Aug 14, 2023
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

  1. I think it's worth doing a final search/replace for removing the residual hardcoded http://127.0.0.1:4000/graphql addresses in favor of the FUEL_NETWORK_URL constant.

  2. We should not postpone the resolution of the broken tests since it can hide critical problems with the taken approach. Addressing and uncommenting those should also fix the failing checks of the next point below.

  3. There are a couple of failing checks here (the last ones), probably due to the commented-out tests mentioned above:

    screenshot

    checks

Torres-ssf
Torres-ssf previously approved these changes Sep 19, 2023
arboleya
arboleya previously approved these changes Sep 19, 2023
@Torres-ssf Torres-ssf merged commit b3b59e2 into master Sep 19, 2023
8 checks passed
@Torres-ssf Torres-ssf deleted the dp/provider-consensus-params-cache branch September 19, 2023 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Issue is a feature
Projects
None yet
7 participants