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: Support Selectable Write Ledger #2339

Merged
merged 10 commits into from
Aug 2, 2023

Conversation

shaangill025
Copy link
Contributor

@shaangill025 shaangill025 commented Jul 21, 2023

@shaangill025 shaangill025 requested a review from esune July 21, 2023 15:57
Copy link
Contributor

@esune esune left a 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:
Copy link
Contributor

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.

Copy link
Contributor Author

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/multiple_ledger/indy_manager.py Outdated Show resolved Hide resolved
Comment on lines 812 to 817
"/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),
]
)
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@dbluhm dbluhm added this to the Version 0.10.0 milestone Jul 25, 2023
@shaangill025 shaangill025 marked this pull request as ready for review July 25, 2023 20:30
Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
esune
esune previously approved these changes Jul 27, 2023
Copy link
Contributor

@esune esune left a 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?

dbluhm
dbluhm previously approved these changes Jul 28, 2023
aries_cloudagent/indy/sdk/profile.py Outdated Show resolved Hide resolved
Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
@shaangill025 shaangill025 dismissed stale reviews from dbluhm and esune via bcb1e4d July 28, 2023 16:03
dbluhm
dbluhm previously approved these changes Jul 28, 2023
Copy link
Contributor

@usingtechnology usingtechnology left a 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.

Multiledger.md Outdated Show resolved Hide resolved
Multiledger.md Outdated Show resolved Hide resolved
@esune
Copy link
Contributor

esune commented Jul 28, 2023

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 /ledger/{ledger_id}/set-write-ledger would be used by each tenant to set which ledger they want to write to.

@shaangill025
Copy link
Contributor Author

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 /ledger/{ledger_id}/set-write-ledger would be used by each tenant to set which ledger they want to write to.

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.

@usingtechnology
Copy link
Contributor

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 <gill.shaanjots@gmail.com>
@shaangill025 shaangill025 dismissed stale reviews from usingtechnology and dbluhm via 4f1637b July 28, 2023 17:53
@shaangill025
Copy link
Contributor Author

shaangill025 commented Jul 28, 2023

Thanks @shaangill025 and @esune. Is that tenant selection permanent? Maybe I am missing something, but it appears that the selection isn't persisted.

It should be, it basically binds BaseLedger instance based upon selection to the Tenant profile.

@shaangill025
Copy link
Contributor Author

@usingtechnology Tenant write ledger selection persistence should be working now.

@sonarcloud
Copy link

sonarcloud bot commented Aug 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@dbluhm dbluhm enabled auto-merge August 2, 2023 16:34
@dbluhm dbluhm merged commit a71c12b into hyperledger:main Aug 2, 2023
9 checks passed
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.

multi-ledger write failure on register-nym
4 participants