feat: unknown module
#4399
feat: unknown module
#4399
Conversation
dpc
force-pushed
the
24-02-22-unknown-module
branch
from
6b7ac15
to
d682979
Compare
fedimintd/src/fedimintd.rs
Outdated
| if std::env::var_os(FM_USE_UNKNOWN_MODULE_ENV).is_some() { | ||
| s.with_module(UnknownInit) | ||
| } else { | ||
| s | ||
| } |
There was a problem hiding this comment.
I'm not a fan of branching like this in production code just to make tests work. Aren't rust integration tests enough? If we really want devimint tests we can build a separate testing fedimintd with this extra module.
There was a problem hiding this comment.
Having separate binaries seem cumbersome for integrations and alike. There's really nothing hacky or insecure about this module, it's a proper module that... is not very useful in practice. If you don't like branching, I could just make fedimintd produce this module unconditionally. :D
I bet eventually default Fedimint will ship with some optional modules anyway, or even the current modules will become opitonal, and users will be choosing them at DKG. It's just we don't have this API surface yet, so an env var is used.
dpc
force-pushed
the
24-02-22-unknown-module
branch
from
d682979
to
5d4a646
Compare
dpc
force-pushed
the
24-02-22-unknown-module
branch
from
05db0ad
to
b4fc176
Compare
| @@ -11,6 +11,9 @@ versions=( "${@:-${default_versions[@]}}" ) | |||
|
|
|||
| # signal to downstream test scripts | |||
| export FM_BACKWARDS_COMPATIBILITY_TEST=1 | |||
| # previous versions did not have unknown module so can't support it, | |||
There was a problem hiding this comment.
@bradleystachurski Food for thought: we will need to deal with all sorts of incompatibilities between versions.
There was a problem hiding this comment.
Yeah, I'd anticipate we'll have a lot of conditional logic in devimint based on versions similar to 18f00a8 (#4247)
There was a problem hiding this comment.
It might be useful to come up with a system that'll allow us to easily cleanup conditional logic that's no longer needed in devimint.
e.g. // FIXME: deprecated v0.2.1 so we can grep for a version when it's reached EOL
There was a problem hiding this comment.
馃 But we should make it an issue and brainstorm a little.
|
Hey, it detected one legitimate bug right away. |
|
#3548 will be a good one to test with this |
That was the one it found, I just pushed it as a separate PR, as it's not clear when we'll land this and how yet. |
| .clone() | ||
| .decoded() | ||
| .map(|decoded| decoded.to_json().into()) | ||
| .unwrap_or(serde_json::Value::Null), |
There was a problem hiding this comment.
Eventually we might want to display the raw binary instead of null?
dpc
force-pushed
the
24-02-22-unknown-module
branch
3 times, most recently
from
efe987a
to
c088794
Compare
dpc
force-pushed
the
24-02-22-unknown-module
branch
from
c088794
to
6b5f0e8
Compare
fedimint-core/src/core.rs
Outdated
| @@ -140,6 +140,7 @@ pub const MODULE_INSTANCE_ID_GLOBAL: u16 = u16::MAX; | |||
| pub const LEGACY_HARDCODED_INSTANCE_ID_LN: ModuleInstanceId = 0; | |||
| pub const LEGACY_HARDCODED_INSTANCE_ID_MINT: ModuleInstanceId = 1; | |||
| pub const LEGACY_HARDCODED_INSTANCE_ID_WALLET: ModuleInstanceId = 2; | |||
| pub const LEGACY_HARDCODED_INSTANCE_ID_UNKNOWN: ModuleInstanceId = 3; | |||
There was a problem hiding this comment.
Is the comment above still relevant?
// Note: needs to be in alphabetical order of ModuleKind of each module,
// as this is the ordering we currently hardcoded.
| @@ -113,6 +113,19 @@ impl Fixtures { | |||
| self | |||
| } | |||
|
|
|||
| pub fn with_server_only_module( | |||
There was a problem hiding this comment.
Would this make sense to use for other fixtures instead of only ln-gateway?
There was a problem hiding this comment.
Yes, but with very diminishing returns.
dpc
force-pushed
the
24-02-22-unknown-module
branch
from
6b5f0e8
to
bdadb50
Compare
We would like to make sure all client code can gracefully handle Federations with modules that the client doesn't support.
The laziest way to do it is to add a new dummy module that has only server side implementation and then instantiate it liberally in tests. This should exercise code paths handling such case without any 馃 downsides.
Easy to test with
just mprocs: