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

Update Namada Specs #333

Merged
merged 3 commits into from
Oct 24, 2022
Merged

Update Namada Specs #333

merged 3 commits into from
Oct 24, 2022

Conversation

arafey
Copy link
Contributor

@arafey arafey commented Aug 15, 2022

Recommendations

This PR implements the following recommendations:

1 Index

General

  • In general, we should provide links for terms which might be unfamiliar to the reader. For example, the index plunges into describing Namada using many terms which a developer who is new to blockchain (or even someone who is experienced with blockchain development but on another platform) may be unfamiliar with (e.g., Sapling circuit, Tendermint BFT consensus, multi-asset shielded pool). We should provide links to such concepts. These could be either external (such as a link to the Tendermint project) or internal (such as a link to MASP section of the specs or MASP Specification paper).

Technical

  • The question "How does Namada relate to Anoma?" is also potentially confusing without context. The reader may not know what exactly Anoma is or why this question is relevant so early in the docs. We should provide a link to the Anoma project.
  • Fractal instances are mentioned repeatedly but not precisely defined. We should draw on the definition of fractal instances in the v2 whitepaper.
  • We should also more clearly define the Anoma network and how it is distinct from the Namada network.
  • We should clarify further that the rationale for varied fractal instances comes for the philosophy of heterogenous security described in the new white paper

Stylistic

  • Head the first section, "What is Namada?"
  • The Raison d'etresection is clearer, but the last paragraph could likely be removed, as it is more of a rationale for intra-Heliax purposes than something an external reader necessarily needs to know.

2.1 Consensus

Stylistic

  • We should consolidate this as it makes the page feel unfinished. We could also elaborate a bit more on the consensus logic or link elsewhere to further reading – the description feels very brief, almost not worthy of its own page.

2.2 Execution

Stylistic

  • We should link directly to EndBlock vs DeliverTx in the ABCI interface so that the reader can easily examine the difference under the hood
  • The clause "which has not been fully finished and hence is out-of-scope…" is unneeded (feels like an internal note)
  • The transaction whitelist link just refreshes the page, which is unexpected
  • The protocol section link is dead (redirects to https://docs.anoma.network/master/specs/ledger.html#the-protocol).
  • VPs are well-defined when they are defined, but should be linked to their definition in the new white paper (as they're referenced multiple times before their definition)

2.3 Governance

Stylistic

  • We could wrap long text blocks to make them easier to read (here and elsewhere) (already done?)
  • Validator/delegator referenced here but not defined until 5.3.1
  • Nit – typos (Pos, gender for governance, transfer misspelled, closed code blocks brackets hanging at page end)
  • In general, this section could do with some light editing to make sentences more succinct. For example, "For each ended proposal with a positive outcome, it will refund the locked funds from GovernanceAddress to the proposal author address (specified in the proposal author fund)" is somewhat wordy, compared to something like "A successful proposal will refund funds from GovernanceAddress to the <type of proposal author address>".
  • Supported interfaces should have links (e.g., to Wallet docs)

2.4 Default Account

General

  • This should be included in the governance section

2.5 Multisignature account

General

  • This should be included in the governance section

2.6 Fungible Token

General

  • This should be included in the governance section

3.1 Ledger integration

General

  • In general, this section often reads as if written for internal consumption only and needs a tonal review as a result. For example, the section "The shielded pool needs the ability…each block" is unexpected as it sounds like it is referring to a feature which needs to be added to the specs and a potential way of adding it. There are question marks, references to needing more input, and even a reference to the fact that "the asset's VP is not run as described above", which seems to admit knowledge that the specs themselves are incorrect. There are also mentions of what the spec must include in future versions (for example, under client capabilities) and heavy use of "should", so that the reader is unsure what exists in current vs future versions of MASP and whether running a command will work or not.

Technical

  • Do we check that an amount exceeding the gas fees is present in a transparent account before we execute an unshielding transaction? (I'm assuming yes from the way that it is written).

Stylistic

  • Unclear why Anoma "established account" is referred to in quotes
  • Multiple references to Anoma which should likely refer to Namada instead (ie Anoma account instead of Namada account)
  • Dead link for Multi-Asset Shielded Pool Specification (https://raw.githubusercontent.com/anoma/masp/main/docs/multi-asset-shielded-pool.pdf)
  • Nit – strange formatting of italicized outputdescription, typo on Specification link and in additionally
  • bellman::groth16 should have an external link

3.2 Asset type

General

  • Similar to 3.1, it is unclear if this section describes an existing or future name schema. There are also ?s, internal notes, use of probably/should, and internal questions (already mostly done)
  • This section, as 3.1, is highly technical, but this may be difficult to avoid in the case of MASP as a whole

3.3 Burn & mint

Stylistic

  • Even allowing for the highly technical nature of the material, this section is not fully clear (e.g., "However, the Nullifier integrity check of the Spend circuit reveals the nullifier of each of these Notes, which removes the privacy of the conversion as the public nullifier is linkable to the allowed conversion").

3.5 Trusted setup

Stylistic

  • Add link to enable sign up for trusted setup.

4. Interoperability

Stylistic

  • As with the other header sections, remove whitespace and/or lengthen slightly

4.1 Ethereum bridge

General

  • Remove the TBD at the bottom regarding how the user is to initialize the storage key.

Stylistic

  • Extract operational note elsewhere

4.2 IBC

Stylistic

  • Multiple links to dead docs.anoma.network docs

5 Economics

General

  • We should probably move this section earlier, since it lays out core concepts that become crucial in other sections. For example, NAM is mentioned earlier, but only formally defined here

5.3.1 Bonding mechanism

General

  • We should consider putting this before section 2 as terms such as Epoch, Validator, Delegator, etc are used there without definition – or else explicitly link to the definitions here.

Stylistic

  • Validator and delegator are even defined too late here, as they are mentioned in Epoched data before their definition below
  • Can remove TBA
  • We should clean up some of the language in this section to make it clearer to the reader (e.g., the sentence starting with "However, because slashes…").
  • Likely want to move "initialization" section before the others since it would occur prior from the user perspective

5.3.2 Reward distribution

Stylistic

  • Wecould consider moving the Slashes section to the top of the slashing page

5.4 Shielded pool incentives

General

  • This section would find a better home among the MASP section or the philosophy, as it is a strong philosophical justification for using shielded pools which, if read before/alongside the reader learning about MASP, would provide important context.

5.5 Public goods funding

General

  • The style here is unusual, as the page seems to take the form of a proposal from an individual regarding the purposes of public goods funding in Namada – remove this language.

6.1 Web wallet

Stylistic

  • The page needs light stylistic editing (e.g., "User can integrated with Ledger hardware wallet", "consists of the an UI")
  • Page includes TBDs - unclear if these are because links are dead and/or because features are unimplemented

Further reading

General

  • We should include more links here – for example, to the Anoma v2 whitepaper when made public, to Anoma Discord, Heliax website, Anoma source code, Anoma Medium page, etc
  • Note – linked Namada.net contains many phrases we should likely pull into the specs, such as "Privacy should be default and inherent in the systems we use for transacting" and the descriptions of the main actions users can take – earning rewards, retaining privacy of assets, and contributing to the overall privacy set (to go in the new "What users can do" section in the intro).

@arafey arafey requested a review from cwgoes August 15, 2022 07:43
cwgoes
cwgoes previously approved these changes Aug 17, 2022
Copy link
Collaborator

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK modulo comment

documentation/specs/src/base-ledger/core-concepts.md Outdated Show resolved Hide resolved
@tzemanovic
Copy link
Member

there's couple broken links - see the "Build docs" CI step. Nvm the errors in "build and test" - it's just because of the older base

@tzemanovic tzemanovic marked this pull request as ready for review October 21, 2022 16:05
@cwgoes cwgoes merged commit 43c5248 into main Oct 24, 2022
@cwgoes cwgoes deleted the abuzer/specs-update branch October 24, 2022 07:18
phy-chain pushed a commit to phy-chain/namada that referenced this pull request Mar 1, 2024
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.

3 participants