Network is a global singleton

[andywer]

... and that's a quite big no-no, especially in JS land 😕

Consider this use case (we have a similar situation as we speak):
There is a package X using stellar-sdk and a package Y using stellar-sdk as well, but a slightly different version.

If Y is a dependency of X or if X and Y are both being used in another package Z then we will end up with a nasty situation. The Network singleton that X imports from stellar-sdk/stellar-base won't be the same instance that Y has, since they each depend on a different version (two different stellar-base in the node_modules/ tree).

Thus you can run Network.useTestNetwork() in X and still get a No network selected. error in Y. Same happens when npm link-ing packages during development, even if all packages require the same version.

Proposal

Is there actually a need to have this stateful Network singleton? I would favor a solution based on passing the pubnet/testnet switch to the Server constructor, since horizon servers won't suddenly change the network they are running on.

import { Network, Server } from 'stellar-base'

const horizon = new Server('https://horizon-testnet.stellar.org/', { network: Network.testnet() })

Sorry for the wall of text 😉

[sgehrman]

agreed. I have a test app that talks to different networks, some test, some live. Kind of a pain to have to worry about this.

[morleyzhi]

Taking some notes here for someone in the future to figure out what it'd take to do this.

Places that check for the current network:

• transaction.js (uses as a part of the signature base)
• keypair.js (uses as a seed for generating the network master key)

It should be easy enough for Keypair.master() to accept a variable for the network key. Transactions would need to be passed the current network though. So the best course of action would be to issue a console warning when Keypair.master() or new Transaction() are called without the network, and then in a later version to turn that warning into an error.

[Akuukis]

What's the status of this? Can I take this to work on?

[abuiles]

@Akuukis > What's the status of this? Can I take this to work on?

Go for it! what do you have in mind? I think it would be a good idea to follow @morleyzhi's suggestion to do it in a way such as we can introduce a warning and then do the deprecation later.

[Akuukis]

@abuiles, yes, with deprecation warning!

There's another issue to discuss - how to handle network passphrase within Transaction class. Options:

1. Pass at each method explicitly (see feat: deprecate Network, use explicit passphrase (1) #206)
2. Pass once at constructor (feat: deprecate Network, use explicit passphrase (2) #207)
3. Use getter/setter

The problem with 1. is that it's not intuitive. For me, I think of Transaction as either coming from specific network, or crafted for specific network (because state of ledger matters, especially sequence number). And this design opens a absurd possibility to add signatures targeted for different networks. Furthermore, given 1. option in order to check list of signatures at my favorite helper I would need to pass wrapped-object {tx, passphrase}, that wouldn't be the case with other options. Thus, I'd prefer to go with 2. option, BUT...

The problem with 2. option is that envelope_xdr doesn't contain network passphrase. So, there may be cases I just don't know network passphrase at construction time. Therefore 3. option to be able to set it later, if at all.

So I'd go with 2. or 3. option. What's your thoughts?

[andywer]

Nice to see that this is finally tackled :)

Just my two cents: I think 3 unfortunately also is the least elegant approach, since it's almost impossible to reason about the code if it will run reliably or not if a pretty-much mandatory parameter is only set lazily via a setter.

I do agree with the shortcomings of 1 and 2, though, too. But maybe the shortcomings of 2 are in practice the smallest burden as a Transaction instance does not necessarily need to be a 1:1 alternative representation of a transaction XDR.

The role of the Transaction could perhaps be re-interpreted as the high-level transaction, consisting of the low-level transaction XDR and the metadata of the environment it's supposed to be used with (a.k.a. the network).

[Akuukis]

Please see PR #207 for 2nd option.

[Akuukis]

@abuiles @morleyzhi

What's your thoughts on which option to proceed?

[morleyzhi]

I think the second option makes the most sense, looks good!

[ire-and-curses]

FWIW in the Go SDK we also set the network during construction of a Transaction. But it can be set later as well.

[abuiles]

@Akuukis thanks! I'll review your PR tomorrow.

[abuiles]

@andywer this has been released in 1.1.0, we still need to update the sdk to use this version.

[andywer]

Awesome stuff! This change will save so many developers from a recurring headache 😄🎉

[abuiles]

@andywer sweet! could you give it a try?

[andywer]

@abuiles Sorry, didn't find the time yet, but I will try to give it a short in the next few days!