-
Notifications
You must be signed in to change notification settings - Fork 516
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: Support Selectable Write Ledger #2339
Feat: Support Selectable Write Ledger #2339
Conversation
shaangill025
commented
Jul 21, 2023
•
edited
Loading
edited
- resolve multi-ledger write failure on register-nym #2168
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how the changes generalize how we handle the configured ledgers and open the door to -potentially - having more than one ledger set to write mode in the future.
I am wondering though if, given the current assumption that there will only ever be one write ledger active at any given time (and there is no way to specify which ledger to target when creating a transaction that needs to be written, as far as I know), we should keep some checks to prevent users from having more than one writeable ledger at the same time.
if is_write_ledger and write_ledger_set: | ||
raise ConfigError("Only a single ledger can be is_write") | ||
elif is_write_ledger: | ||
if is_write_ledger: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change going to cause issues with single-tenant agents being able to declare more than one write ledger in the config file? I believe currently the assumption is that there only ever is one "write" ledger at any given time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The meaning of is_write
has changed with this PR. For example:
- id: localVON
is_production: false
genesis_url: 'http://host.docker.internal:9000/genesis'
- id: bcorvinTest
is_production: true
is_write: true
genesis_url: 'http://test.bcovrin.vonx.io/genesis'
- id: greenlightDev
is_production: true
is_write: true
genesis_url: 'http://dev.greenlight.bcovrin.vonx.io/genesis'
is_write
property means that the ledger is write configurable. With reference to the above config example, both bcorvinTest
and greenlightDev
ledgers are write configurable. By default, on startup bcorvinTest
will be the write ledger as it is the topmost write configurable production ledger. Using PUT /ledger/{ledger_id}/set-write-ledger
endpoint, either greenlightDev
and bcorvinTest
can be set as the write ledger.
aries_cloudagent/ledger/routes.py
Outdated
"/ledger/multiple/get-write-ledger", get_write_ledger, allow_head=False | ||
), | ||
web.put("/ledger/multiple/{ledger_id}/set-write-ledger", set_write_ledger), | ||
web.get( | ||
"/ledger/multiple/get-write-ledgers", | ||
get_write_ledgers, | ||
allow_head=False, | ||
), | ||
web.get("/ledger/multiple/config", get_ledger_config, allow_head=False), | ||
] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need the /multiple/
path added? Whether the agent is connected to one ledger or more than one, I think the endpoints should still return information. The ledger_id
can be used as path parameter where the operation requires one ledger instance to be identified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/multiple/
has been removed. In case, multi-ledger config is not provided and single-ledger is applicable then the endpoint's response will return ledger_id
as default
. This is not applicable for GET /ledger/config
, it will return a Multiple ledger support not enabled
error.
ecb8be6
to
3e15149
Compare
Signed-off-by: Shaanjot Gill <[email protected]>
Signed-off-by: Shaanjot Gill <[email protected]>
Signed-off-by: Shaanjot Gill <[email protected]>
Signed-off-by: Shaanjot Gill <[email protected]>
8c86657
to
f9bb910
Compare
Signed-off-by: Shaanjot Gill <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @shaangill025 , this looks good!
@andrewwhitehead , @dbluhm and @usingtechnology could you please take a look and confirm I did not miss somethign important?
Signed-off-by: Shaanjot Gill <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling in documentation (bcorvinTest -> bcovrinTest) to update.
I have a bigger question... I don't see anything about multitenancy? Is that coming later? I would assume that each tenant could potentially write to a different ledger.
I believe |
There will be a default write ledger set based on the multi-ledger config provided for all tenants. The tenants can then use the above endpoint to set different write ledger. |
Thanks @shaangill025 and @esune. Is that tenant selection permanent? Maybe I am missing something, but it appears that the selection isn't persisted. |
Signed-off-by: Shaanjot Gill <[email protected]>
4f1637b
It should be, it basically binds |
Signed-off-by: Shaanjot Gill <[email protected]>
…t-python into multi_ledger_write
@usingtechnology Tenant write ledger selection persistence should be working now. |
Kudos, SonarCloud Quality Gate passed! |