-
Notifications
You must be signed in to change notification settings - Fork 46
Support for granting ValidatorLicense via SV UI #2905
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
base: main
Are you sure you want to change the base?
Support for granting ValidatorLicense via SV UI #2905
Conversation
|
@dfordivam I'm confused. I thought we agreed that we're breaking the work into a series of small PRs that one can effectively review. It's very hard to review thoroughly a PR of this magnitude that's a code dump of your complete work. |
|
@isegall-da I would have preferred to change the "base" branch, such that the daml changes included in this branch are not shown in this PR. (I could not set the base to my branch as it is on a fork). |
Signed-off-by: Divam <[email protected]>
Signed-off-by: Divam <[email protected]>
Signed-off-by: Divam <[email protected]>
Signed-off-by: Divam <[email protected]>
Signed-off-by: Divam <[email protected]>
Signed-off-by: Divam <[email protected]>
Signed-off-by: Divam <[email protected]>
Signed-off-by: Divam <[email protected]>
Signed-off-by: Divam <[email protected]>
Signed-off-by: Divam <[email protected]>
Signed-off-by: Divam <[email protected]>
Signed-off-by: Divam <[email protected]>
Signed-off-by: Divam <[email protected]>
3c79212 to
1e3dc30
Compare
Why can't you base the branch on your own branch? You can stack multiple PRs one on top of the other, thus breaking the work into chunks that a reviewer can digest and review effectively. |
|
Alternatively, you can rework the git history in the branch such that it's a series of self-contained commits that one can review one at a time. |
So its just a Github limitation, my branch is on the fork, and the base branch has to be on |
|
I'll review tomorrow morning my time. |
|
@dfordivam : on
wdyt about adding 'default: 1.0' when the weight is not set? I'd also suggest to invert the double-negation: call the column "Operator" and add a checkmark if its an operator license. |
meiersi-da
left a comment
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.
Nice work @dfordivam !
Looks good up to the testing changes and small pieces of missing functionality. Let me know when you've addressed that so I can do a second review.
| } | ||
|
|
||
| @Help.Summary("Grant a ValidatorLicense to a validator party (via admin API)") | ||
| def grantValidatorLicense(partyId: PartyId): Unit = |
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.
what about setting a non-standard weight?
| weight: | ||
| description: | | ||
| Weight of the activity records produced by this license. Determined | ||
| through SV votes. If not set, weight is 1.0 |
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.
wdyt about making the field required and always setting the default value. Thereby limiting the propagation of the default? No need for the clients to implement the defaulting, or is there?
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.
By always returning the default value, instead of missing field/null, we are losing information which may be useful for the clients. I can set it if required.
| kind: | ||
| description: | | ||
| Type of license: OperatorLicense or NonOperatorLicense. If not set, | ||
| kind is OperatorLicense |
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.
Same here: resolve the default on the server.
| Optional.empty(), // optTotalValidatorFaucetCoupons (deprecated) | ||
| Optional.of( | ||
| java.math.BigDecimal.valueOf(validatorLivenessActivityRecords) | ||
| ), // optTotalValidatorLivenessActivityRecords |
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 can't find the corresponding change to the store method to retrieve the sum of weights. Am I looking in the wrong place?
I'd have expected:
- an adjusted
SvDsoStoreTest - an adjusted integration test for validator faucet coupons that tests a non-default weight license
| GrantValidatorLicenseRequest: | ||
| type: object | ||
| required: | ||
| - party_id |
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.
weight
| } | ||
| import scala.jdk.OptionConverters.* | ||
|
|
||
| class SvValidatorLicenseIntegrationTest extends SvIntegrationTestBase { |
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.
This start a 4SV topology, which is unnecessarily expensive for this kind of test. I know there are other tests that do the same, but they should ideally also be adjusted.
Having had a peek at the tests we have I suggest: you merge your new test with the one in https://github.com/hyperledger-labs/splice/blob/main/apps/app/src/test/scala/org/lfdecentralizedtrust/splice/integration/tests/SvMergeDuplicatedValidatorLicenseIntegrationTest.scala (probably want to use your name), but use the environment setup from there:
Lines 25 to 33 in 63de58e
| override def environmentDefinition | |
| : org.lfdecentralizedtrust.splice.integration.EnvironmentDefinition = | |
| EnvironmentDefinition | |
| .simpleTopology1Sv(this.getClass.getSimpleName) | |
| .addConfigTransforms((_, config) => | |
| updateAutomationConfig(ConfigurableApp.Sv)( | |
| _.withPausedTrigger[MergeValidatorLicenseContractsTrigger] | |
| )(config) | |
| ) |
|
|
||
| val dsoParty = sv1Backend.getDsoInfo().dsoParty | ||
| val sv1Party = sv1Backend.getDsoInfo().svParty | ||
| val validatorParty = allocateRandomSvParty("test-validator", Some(100)) |
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.
that sounds off. This party gets allocated on sv1, which means we might not discover problems due to non-local reads.
I'd suggest to instead just grant the license to alice
| class SvValidatorLicenseIntegrationTest extends SvIntegrationTestBase { | ||
|
|
||
| "grant validator license via admin API" in { implicit env => | ||
| initDso() |
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.
| initDso() |
having to manually start nodes should only be required in special circumstances ==> it's a bit of a test smell.
| val validatorParty = allocateRandomSvParty("test-validator", Some(100)) | ||
|
|
||
| // Verify no license exists initially | ||
| val initialLicenses = sv1Backend.participantClientWithAdminToken.ledger_api_extensions.acs |
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.
read through alice's node
| </Button> | ||
| </DisableConditionally> | ||
|
|
||
| {grantLicenseMutation.isError && ( |
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.
@isegall-da : the code looks OK, but I have done too little frontend work to be sure. Could you have a peek or delegate having a peek?
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.
We're missing weight also in the UI, but given that the endpoint doesn't have one I guess that makes sense for now (happy for it to be added in a followup PR). Otherwise, looks good
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.
Setting a weight when granting license is prohibited by the CIP, the weight always defaults to none. Hence daml, scala and UI does not have an option to specify it.
|
|
||
| const VALID_PARTY_ID_REGEX = /^[^-]+-[^-]+-\d+$/; | ||
| const VALID_PARTY_HINT_REGEX = /^[^-]+-[^-]+-\d+$/; | ||
| const VALID_PARTY_ID_REGEX = /^[a-zA-Z0-9_-]+::[a-zA-Z0-9_-]+$/; |
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.
@meiersi-da is the fingerprint part of of constant length? Can we strengthen this check to be an exact match on it?
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.
it currently is of constant length and hex encoded, but I wouldn't be surprised if it changes in the future. Would you be OK with having to change this regex in that case?
Probably makes sense to check here, as dropping a fingerprint digit is quite easy when manually copying a party-id. I'll ask the Canton team about the guarantees.
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.
Yes, I think I'd be inclined to have this assert tightly on the current constraints, and potentially relax that in the future if needed. Quite easy to get a copy-paste of a long blob wrong.
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.
@dfordivam : I've checked with the Canton team. Here's the regex they propose to use:
^[A-Za-z0-9_-]{1,185}::[A-Fa-f0-9]{68}$ please switch to that one.







Fixes #2904
ValidatorLicensevia SV UI and console.Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines