I have no clue, but I’ll happily let my colleague answer 
If by being compatible you mean whether the NNS Dapp uses the SNS ledger fee when making SNS transactions, the answer is yes.
Or do you mean something else?
Only the NNS ledger fee is hardcoded in NNS Dapp (which we will change also). The rest of the fees come from the canisters.
2 Likes
@levi Just a quick update, still following up on how we could accept changes, and getting opinions on the changes themselves, and what is necessary from a licensing perspective etc so that the project is not impacted negatively by those issues. I’ll follow up when I have something more definite to share.
3 Likes
Thanks @peterparker, @lmuntaner, cool, compatible means that the nns-dapp fetches the sns ledger transfer fees from the sns ledger canisters at least once at the beginning of each user-session (even once per day is fine but not the best). If that is the case then we are good. Let me know if this definition of compatible is different to the one you were thinking.
1 Like
@msumme I am with the focus on the facts of these code-changes. It can help the community if the DFINITY foundation engineers share their knowledge about the following fact:
Does the code-changes of this proposal create the functionality for a sns to update it’s ledger-transfer-fee and it’s fee-collector-account by the sns-proposal-type: ManageLedgerParameters.
This is the fact that keeps this proposal on the point with it’s goal.
1 Like
That the same definition of compatible I had in mind 
1 Like
Hi Levi. Yes, the code changes do seem to accomplish the intended purpose. If we were to take it as a proof of concept, it could be put into the queue as a feature.
You may have noticed, in the Github IC repo, some files with Gitlab CI processes? Our internal CI/CD systems are currently running on Gitlab. It’s in process to migrate to Github, as that is now technically feasible. If you inspect the commits, you might also notice they point to merge requests that don’t exist in Github.
The reason I’m pointing this out is that accepting the contribution is not yet something we can do automatically. I have been working on outlining the process for accepting contributions before we get to the state that we can simply merge a PR in Github (there’s not a definite date that I know of where that will be finished, but there has been work done to help make that happen).
If the PR you created were to eventually get merged, it would need some unit tests and state machine tests. Additionally, there is an agreement contributors have to sign so that the code is appropriately licensed for the IC project (which normally is part of the Github process for other repositories that already accept contributions) which would have to be done.
Would you be interested in trying to continue the work to the point that it could be merged? Or are you rather hoping for DFINITY engineers to finish the remaining work of writing tests, etc?
6 Likes
I take full countability for the code of this feature. The tests are now built into the code-changes branch. The state-machine tests are in this file.
Run the tests:
-
mkdir sns-manage-ledger-parameters && cd sns-manage-ledger-parameters -
git clone -b levifeldman-sns-manage-ledger-parameters --single-branch https://github.com/levifeldman/ic.git && cd ic -
./gitlab-ci/container/container-run.sh -
bazel test --config=local //rs/sns/integration_tests:integration_test_src/manage_ledger_parameters
There is a test at GitHub - levifeldman/test-manage-ledger-parameters written in Dart. Commands for running this test are here:
Must have linux, dfx, and Dart for this test.
Run the test:
-
mkdir -p mlp-test && cd mlp-test -
git clone -b levifeldman-sns-manage-ledger-parameters --single-branch https://github.com/levifeldman/ic.git && cd ic -
./gitlab-ci/container/build-ic.sh -c --no-release -
cd .. && git clone https://github.com/levifeldman/test-manage-ledger-parameters.git && cd test-manage-ledger-parameters -
dfx start --background --artificial-delay=0 -
dart pub get && dart d.dart $(pwd)/../ic/artifacts/canisters
The CLA - contributors license agreement is set up already for my github account by my contributions to other dfinity github repositories.
3 Likes
Record of this is here: cla: @levifeldman · Issue #15 · dfinity/cla · GitHub
3 Likes
@levi Apologies for the delay, but I believe I’ve worked out a process for accepting your contribution.
Essentially, I will be mirroring your commits internally, then merging in a way that would keep your authorship on them. Since you’ve agreed to the CLA, we can proceed this way in this case, and see how it goes.
A more robust process is being developed, but I wanted to try to honor the work you’ve done, and see if we can get it across the finish line.
I’ll make some comments on your PR in Github.
4 Likes
Hi @msumme, Cool, that sounds good. I’ll continue the technical conversation on the GitHub PR. For those following, I’ll post here for updates on this feature.
2 Likes
Hi forum people, while the PR for the ManageLedgerParameters sns proposal type is merging into the master, it has come up in the forum in the conversation here that the fee-collector functionality that is implemented in the ledger is not standardized and therefore ICRC compatible ledger-readers will not be able to compute the correct account balances by looking at the transaction logs. Due to this conundrum, I am now taking away the change_fee_collector field from the ManageLedgerParameters sns proposal type. If the ledger’s fee-collector functionality does get a standard, then we can put the option back.
The ManageLedgerParameters SNS proposal type now contains the single functionality for a change of a sns-ledger’s transfer_fee.
3 Likes
Congratulations! ！！
Great accomplishment for a grueling process！
Looking forward to more and more people and MORE being able to follow you！
…
@levi This MR has been merged! It will be deployed with the next SNS deployment.
Thanks for all the effort that was put into this. Hopefully your next contribution will be easier!
(when the tools and infrastructure is available on Github).
4 Likes
Hey, Awesome! You are welcome, I’m glad it works out. Thanks to you and your team for helping this get merged. I’m looking forward.
2 Likes
The ManageLedgerParameters feature is now live on the latest sns-governance-canister-module. The proposal has passed with the success. SNSs can now change their ledger’s transfer fee.
3 Likes